From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 9031B5FCBFC; Fri, 29 Sep 2023 11:24:33 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9031B5FCBFC DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1695975873; bh=w2e/ZGOWlFFVMUgHTxznwJqXbHFhObQ1A265nSv1EHU=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=Tz0HD+jN0U0kE3N/sbHOVZxUFBgNkdkkETtprbA3rrABar4Sv7nT+12DeHv7ZXoXL lISOjad8umDzago5a4UxL2lOXZ+lPvlfZ6liYNLeXzOTRZ+7iD1glu2VXUcvYcdMa8 j8QFSerIQM549nI5Su4ro6kma7R23G6FcsSoEgSg= Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [95.163.41.87]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9F7335FC8F2 for ; Fri, 29 Sep 2023 11:24:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9F7335FC8F2 Received: by smtp51.i.mail.ru with esmtpa (envelope-from ) id 1qm8nX-00HXuy-0G; Fri, 29 Sep 2023 11:24:31 +0300 Date: Fri, 29 Sep 2023 11:24:22 +0300 To: Sergey Bronnikov Message-ID: References: <26636e3dcb8ade3bdc23d9544f9a7e2ffe4322b2.1695711374.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <26636e3dcb8ade3bdc23d9544f9a7e2ffe4322b2.1695711374.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9635F647A8D3EE72CFDECEAD65197480425EDE993E8E0D7C1182A05F53808504018096BCD08C7D72B969B9619181ADE911D95EAF222EFC677BB7139EAF086C427 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75DF2B1F23425CAE5EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374146C1300AF3DC028638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D889BA15AD6EF685A7AC7CB8ACE765B1E9117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC55D5BE2F85BDEC5FA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD1828451B159A507268D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE9647ADFADE5905B17C6FCE95544A9834D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE3632EDEA9CD5989A36E0066C2D8992A16C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006371F24DFF1B2961425731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5D0EA46E03E59D0769C269EA92053D7DDAAEAD3CD0A4397B6F87CCE6106E1FC07E67D4AC08A07B9B067F1C1C3ABB44F3ACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF5EB5C64FD30D2F95F985A4F7A16143EBE82658725E4CE7A6BC60670966C6A2124C7498080C0F87FCD499C240B04E5D6F6A31A32FB5580E5F2C1DE393C361E1EEA74DFFEFA5DC0E7F02C26D483E81D6BE64ACE4A408B72B61B0CA6F94E606A667A52EF62A646584F811BD90D3D42C882D43082AE146A756F3 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojMN56O+Bp+qzDlrEqgvwhOQ== X-Mailru-Sender: EFA0F3A8419EF21686A196E93EBC065A822D823C2B04232A969B9619181ADE915AB84C596710711004C9FB44FCBCE9EE92D99EB8CC7091A7ECEABDC5717908DEF544888E8238EB4872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix lua_concat(). X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the patch! Please consider my comments below. On Tue, Sep 26, 2023 at 09:56:31AM +0300, Sergey Bronnikov via Tarantool-patches wrote: > From: Sergey Bronnikov > > Reported by Mathias Westerdahl. > > (cherry picked from commit 633f265f67f322cbe2c5fd11d3e46d968ac220f7) > > Lua 5.1 Reference Manual [1] defines a function `lua_concat`, that: > > > void lua_concat (lua_State *L, int n); > > > > Concatenates the n values at the top of the stack, pops them, and leaves > > the result at the top. > > Without the patch `lua_concat()` behaved incorrectly with userdata with > defined `__concat` metamethod. The problem is GC64-specific. > > Assuming we have three literals and a userdata with defined "__concat" > metamethod on top of the Lua stack: > > 1 [string] > 2 [string] > 3 [string] > 4 [string] > 5 [userdata] <--- top > > On attempt to concatenate *two* items on top Lua stack, `lua_concat()` Typo: s/on top/on top of the/ > concatenates *four* items and leaves result at the top: Typo: s/at the/on the/ > > 1 [string] > 2 [string][string][string][userdata] <--- top > > The problem is in incorrect calculation of `n` counter in a loop in Typo: s/in a loop/in the loop/ > implementation of function `lua_concat`. Without a fix `n` is equal to 3 Typo: s/a fix/the fix/ > at the end of the first iteration and therefore it goes to the next > iteration of concatenation. In a fixed implementation of `lua_concat()` Typo: s/In a fixed/In the fixed/ > `n` is equal to 1 at the end of the first loop iteration, decremented in > a loop postcondition and breaks a loop. Typo: s/a loop/the loop/ > > The patch fixes incorrect behaviour. > > 1. https://www.lua.org/manual/5.1/manual.html > > Sergey Bronnikov: > * added the description and the test for the problem > > Part of tarantool/tarantool#8825 > --- > PR: https://github.com/tarantool/tarantool/pull/9176 > Branch: https://github.com/tarantool/luajit/commits/ligurio/lj-881-fix-concat > Issues: > - https://github.com/LuaJIT/LuaJIT/issues/881 > - https://github.com/tarantool/tarantool/issues/8825 > > src/lj_api.c | 2 +- > .../lj-881-fix-lua-concat.test.c | 116 ++++++++++++++++++ > 2 files changed, 117 insertions(+), 1 deletion(-) > create mode 100644 test/tarantool-c-tests/lj-881-fix-lua-concat.test.c > > diff --git a/src/lj_api.c b/src/lj_api.c > index 05e02029..3bacad33 100644 > --- a/src/lj_api.c > +++ b/src/lj_api.c > @@ -801,7 +801,7 @@ LUA_API void lua_concat(lua_State *L, int n) > L->top -= n; > break; > } > - n -= (int)(L->top - top); > + n -= (int)(L->top - (top - 2*LJ_FR2)); > L->top = top+2; > jit_secure_call(L, top, 1+1); > L->top -= 1+LJ_FR2; > diff --git a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c > new file mode 100644 > index 00000000..464ed27e > --- /dev/null > +++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c > @@ -0,0 +1,116 @@ > +#include > + > +#include "lua.h" > +#include "lauxlib.h" > + > +#include "test.h" > +#include "utils.h" > + > +/* > + * This test demonstrates LuaJIT's incorrect behaviour, when > + * calling `lua_concat()` with userdata with __concat metamethod. > + * See https://github.com/LuaJIT/LuaJIT/issues/881 for details. > + */ > + > +#define TYPE_NAME_INFO "Info" > + > +typedef struct Info > +{ > + int value; > +} Info; > + > +static void lua_pushinfo(lua_State *L, Info *info) > +{ > + Info *infop = (Info *)lua_newuserdata(L, sizeof(Info)); > + *infop = *info; > + > + luaL_getmetatable(L, TYPE_NAME_INFO); > + lua_setmetatable(L, -2); > +} > + > +static int INFO_concat(lua_State *L) > +{ > + const char *s = luaL_checkstring(L, 1); > + Info *info = (Info*)luaL_checkudata(L, 2, TYPE_NAME_INFO); > + lua_pushfstring(L, "%s[%s.value=%d]", s, TYPE_NAME_INFO, info->value); > + return 1; > +} > + > +static const luaL_Reg INFO_methods[] = > +{ > + {0, 0} Nit: It is more common to write `NULL` as sentinel. > +}; This table is redundant. > + > +static const luaL_Reg INFO_meta[] = > +{ > + {"__concat", INFO_concat}, > + {0, 0} Nit: It is more common to write `NULL` as sentinel. > +}; > + > +static int lua_concat_testcase(void *test_state) > +{ > + lua_State *L = test_state; > + > + /* Create methods table, add it to the globals. */ > + luaL_register(L, TYPE_NAME_INFO, INFO_methods); > + int methods_idx = lua_gettop(L); The methods table is redundant. > + /* Create metatable and add it to the Lua registry. */ > + luaL_newmetatable(L, TYPE_NAME_INFO); > + > + int metatable_idx = lua_gettop(L); > + /* Fill metatable. */ > + luaL_register(L, 0, INFO_meta); The middle argument is meant to be NULL. > + > + lua_pushliteral(L, "__metatable"); > + /* Duplicate methods table. */ > + lua_pushvalue(L, methods_idx); > + lua_settable(L, metatable_idx); > + lua_pop(L, 2); This whole part can be reduced to this: | /* Create metatable and add it to the Lua registry. */ | luaL_newmetatable(L, TYPE_NAME_INFO); | | /* Fill metatable. */ | luaL_register(L, 0, INFO_meta); | lua_pop(L, 1); | > + > + assert_true(lua_gettop(L) == 0); > + > + Info info; > + info.value = 7; > + > + lua_pushliteral(L, "C"); > + lua_pushliteral(L, "B"); > + lua_pushliteral(L, "A"); > + lua_pushinfo(L, &info); > + > + int top = 4; > + assert_true(lua_gettop(L) == top); > + > + /** > + * void lua_concat (lua_State *L, int n); > + * > + * Concatenates the n values at the top of the stack, > + * pops them, and leaves the result at the top. If n is 1, > + * the result is the single value on the stack; if n is 0, > + * the result is the empty string [1]. > + * > + * 1. https://www.lua.org/manual/5.1/manual.html > + */ > + int n = 2; > + lua_concat(L, n); > + > + const char *str = lua_tostring(L, -1); > + assert_true(top - n + 1 == lua_gettop(L)); We have `assert_int_equal` in `test.h` for that. > + assert_true(strcmp(str, "A[Info.value=7]") == 0); One should never use any of the unrestricted string functions, as they are unsafe. Always use the `n`-limited versions: `strncmp` instead of `strcmp`, `strnlen` instead of `strlen`, etc. Side note: there is a TODO in test.h about `assert_str_equal`, maybe it's a great time to introduce it in this patch. > + > + /* Cleanup. */ > + lua_settop(L, 0); > + > + return TEST_EXIT_SUCCESS; > +} Some general comments about that function: 1. Please mark variables as `const` where possible. 2. C style requires you to put variable declarations at the top of the function. 3. You have three distinct parts in this function: set up procedure, the test itself, and the the tear down. It would be better to split them into three separate functions to improve readability. > + > +int main(void) > +{ > + lua_State *L = utils_lua_init(); > + const struct test_unit tgroup[] = { > + test_unit_def(lua_concat_testcase), > + }; > + const int test_result = test_run_group(tgroup, L); > + utils_lua_close(L); > + > + return test_result; > +} > -- > 2.34.1 > Best regards, Maxim Kokryashkin