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 B4CEF5ECD6B; Tue, 5 Sep 2023 16:00:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B4CEF5ECD6B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1693918838; bh=1RNoSS4ZVL9V3xTxhOrmoK+SqdkQ3Jer4UiSYw1MDTc=; 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=eWVEu643lbKoSjJ45LaraPP6LnXEkgWenbtEsZRCQsbEUK7GL3F3TJ46Q1PSERlC7 HnHWpE3NzIxaoqOrKCk4c58HPrRlNtdPgGLA6iesIIzujYUitAOU27qurDIwnIjy+c aNrAJ+fwNTgE06TBc+1a52W90zXVFuOUmMLDy+i8= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 86C0D5ECD6B for ; Tue, 5 Sep 2023 16:00:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 86C0D5ECD6B Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1qdVfY-0001s8-Ps; Tue, 05 Sep 2023 16:00:37 +0300 Date: Tue, 5 Sep 2023 15:55:53 +0300 To: Sergey Bronnikov Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD96E142CFC92DB15CD43B17B8DE4233CAE5689C6DEE55D6799182A05F538085040F54D74E10AF7D5ABE2581B353A393C7FA5CEB90A87E6E240D03053A0C569D180 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76D34FAA3D8B31588C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE75A6765C746F51968EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BE5CCB53A13BC8DBA4D5774A607CCDCE57E0A69D26F0B1F9BCC7F00164DA146DAFE8445B8C89999728AA50765F7900637602B56D4CB39C6A8389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8A9FF340AA05FB58CF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE33AC447995A7AD181150BA43C84913FC3A03B725D353964B1D471462564A2E1935872C767BF85DA227C277FBC8AE2E8B041BD12FB6B4799375ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5182083B199AF278FE99181F21E6B7FF2BDCB0712BC1DB69EF87CCE6106E1FC07E67D4AC08A07B9B013BDA61BF53F5E1DCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3429499E429979B7C721C700DEC5FB2838D0C82B775A1A76F926297F6B8485B86EC5C21EA353AD52D71D7E09C32AA3244C0F87C19C85F07DFA9ACE989D86874591B018FE5BB746DCD1BAD658CF5C8AB4025DA084F8E80FEBD3202CD0F03380D9577A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj3VBHtC3oXrPjggjoMqfbLA== X-DA7885C5: 267034299469A05E868688CC358AF9882F71C636D46F09D65134F27843261790262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986BE5678DC9351EB74DE2439E31048FFF420FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/2][v2] Followup fix for embedded bytecode loader. 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun 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 31.08.23, Sergey Bronnikov wrote: > From: Sergey Bronnikov > > (cherry-picked from commit e49863eda13d095b1a78fd4ca0fd3a6a9a17d782) > > The patch follows up a previous patch and limits the total size of a > chunk load by `lua_load` with size `LJ_MAX_BUF - 1`. > > Sergey Bronnikov: > * added the description and the test > --- > src/lj_lex.c | 1 + > test/tarantool-c-tests/lj-549-lua_load.test.c | 134 ++++++++++++++++++ I suggest renaming the test to lj-549-lua-load.test.c to be consistent with other tests. > 2 files changed, 135 insertions(+) > create mode 100644 test/tarantool-c-tests/lj-549-lua_load.test.c > > diff --git a/src/lj_lex.c b/src/lj_lex.c > index 6291705f..13495c41 100644 > --- a/src/lj_lex.c > +++ b/src/lj_lex.c > @@ -51,6 +51,7 @@ static LJ_NOINLINE LexChar lex_more(LexState *ls) > if (sz >= LJ_MAX_BUF) { > if (sz != ~(size_t)0) lj_err_mem(ls->L); > sz = ~(uintptr_t)0 - (uintptr_t)p; > + if (sz >= LJ_MAX_BUF) sz = LJ_MAX_BUF-1; > ls->endmark = 1; > } > ls->pe = p + sz; > diff --git a/test/tarantool-c-tests/lj-549-lua_load.test.c b/test/tarantool-c-tests/lj-549-lua_load.test.c > new file mode 100644 > index 00000000..9baa7a1a > --- /dev/null > +++ b/test/tarantool-c-tests/lj-549-lua_load.test.c > @@ -0,0 +1,134 @@ > +#include This include is excess. > +#include Ditto. > +#include > +#include Ditto. > +#include > +#include Ditto. > + > +#include > +#include This include is excess since all libs are opened via utils. > +#include This include is excess since there is no `luaL*` functions or structures usage (and there is no usage of the `LUA_ERRFILE`, `LUA_NOREF`, `LUA_REFNIL`). > + > +#include "test.h" > +#include "utils.h" > + > +/* Need for skipcond. */ > +#include "lj_arch.h" There is no skipconditions, so this include may be dropped. > + > +/* Defined in lj_def.h. */ > +#define LJ_MAX_MEM32 0x7fffff00 /* Max. 32 bit memory allocation. */ > +#define LJ_MAX_BUF LJ_MAX_MEM32 /* Max. buffer length. */ Why don't use `#include "lj_def.h"` instead and mention what we need from it? Reminder: this is kind of unit tests (or these C tests may implement unit test). So, we can include internal libraries, and this is OK for __our C tests__. > + > +/* Defined in lua.h. */ > +/* mark for precompiled code (`Lua') */ > +#define LUA_SIGNATURE "\033Lua" We already included , so this define isn't required. > + > +#define UNUSED(x) ((void)(x)) > + > +/** There is no need in double '*' outside functions (we're not in Kansas anymore. :)) I suggest to be consistent with other tests codebase and use just `/*`. > + * Function generates a huge chunk of "bytecode" with a size bigger than > + * LJ_MAX_BUF. Generated chunk must enable endmark in a Lex state. Nit: Comment line width is greater than 66 symbols. Typo: s/Generated/The generated/ (I'll proceed with the branch verison below.) | static const char * | bc_reader_with_endmark(lua_State *L, void *data, size_t *size) The comment is desirable about the resulting chunk: According the Lua 5.1 Reference Manual: | To signal the end of the chunk, the reader must return `NULL` or set | `size` to zero. So, since this function returns `NULL`, the resulting chunk should be treated as "". Which provides the following bytecode: | "endmark":0-1 | 0000 FUNCV rbase: 1 | 0001 RET0 rbase: 0 lit: 1 This is also avoids test's failure before the patch: we just return earlier: | | if (p == NULL || sz == 0) return LEX_EOF; So, looks like the test doesn't check the patch itself. > + { > + UNUSED(data); > + *size = ~(size_t)0; > + > + return NULL; > + } > + > + static int bc_loader_with_endmark(void *test_state) > + { > + lua_State *L = test_state; > + void *ud = NULL; > + int res = lua_load(L, bc_reader_with_endmark, ud, "endmark"); > + > + /* > + * Make sure we passed the condition with lj_err_mem in the function Nit: Comment line width is greater than 66 symbols. > + * `lex_more`. > + */ > + assert_true(res != LUA_ERRMEM); Maybe it's better to use here codition res == LUA_OK? > + lua_settop(L, 0); > + > + return TEST_EXIT_SUCCESS; > + } > + > + enum bc_emission_state { > + EMIT_BC, > + EMIT_EOF, > + }; > + > + typedef struct { > + enum bc_emission_state state; > + } dt; > + > + /** Typo: s > + * Function returns a bytecode chunk on the first call and NULL > + * and size equal to zero on the second call. Triggers the flag > + * `END_OF_STREAM` in the function `lex_more`. > + */ > + static const char * > + bc_reader_with_eof(lua_State *L, void *data, size_t *size) > + { > + UNUSED(L); > + dt *test_data = (dt *)data; > + if (test_data->state == EMIT_EOF) { > + *size = 0; > + return NULL; > + } > + > + static char *bc_chunk = NULL; > + free(bc_chunk); This free is called only once, when bc_chunk is already NULL. I suggest moving the initialization of the `bc_chunk` to the beginning of the scope and calling `free()` only for the `EMIT_EOF` state (it's also a little bit more readable -- a reader shouldn't remember that `free(NULL)` is OK). > + > + /** Typo: s > + * Minimal size of a buffer with bytecode: > + * signiture (1 byte) and a bytecode itself (1 byte). Typo: s/a bytecode/the bytecode/ Typo: s/signiture/The signature/ > + */ > + size_t sz = 2; > + bc_chunk = malloc(sz); > + /** Typo: s > + * `lua_load` automatically detects whether the chunk is text or binary, Typo: s/binary,/binary/ > + * and loads it accordingly. We need a trace for *bytecode* input, > + * so it is necessary to deceive a check in `lj_lex_setup`, that > + * makes a sanity check and detects whether input is bytecode or text > + * by the first char. Put `LUA_SIGNATURE[0]` at the beginning of the > + * allocated region. Nit: Comment line width is greater than 66 symbols. > + */ > + bc_chunk[0] = LUA_SIGNATURE[0]; > + *size = sz; > + test_data->state = EMIT_EOF; > + > + return bc_chunk; > + } > + > + static int bc_loader_with_eof(void *test_state) > + { > + lua_State *L = test_state; > + dt test_data = {0}; > + test_data.state = EMIT_BC; > + int res = lua_load(L, bc_reader_with_eof, &test_data, "eof"); > + assert_true(res = LUA_ERRSYNTAX); Typo: s/=/==/ But res is indeed `LUA_ERRSYNTAX` for now :). > + lua_settop(L, 0); > + > + return TEST_EXIT_SUCCESS; > + } > + > + int main(void) > + { > + lua_State *L = utils_lua_init(); > + const struct test_unit tgroup[] = { > + test_unit_def(bc_loader_with_endmark), > + test_unit_def(bc_loader_with_eof) > + }; > + > + const int test_result = test_run_group(tgroup, L); > + utils_lua_close(L); > + return test_result; > + } [1]: https://www.lua.org/manual/5.1/manual.html#lua_Reader -- Best regards, Sergey Kaplun