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 47F8F5EF426; Thu, 7 Sep 2023 10:04:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 47F8F5EF426 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1694070275; bh=Fau8ZPRgFo56yss909gSr0fnieroLXhVfOkprT4q8Xc=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=vi7jTjVHBg8NG1tkI6/xLyzD/rdTp55jddI6Dbz03Zvoz5GefhJuwZ6jOZFXxmm8E v9BlLPH3aA00ZJx78KCMQCgtKCFMRmYRAo4oBfj8HuNIUqkuJcMChaiQ7AeJq97xd4 6zLshYhhW6BDoEXpl0i3/M1v41AByTgzyC2LLV38= Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [95.163.41.77]) (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 81F015A5F77 for ; Thu, 7 Sep 2023 10:04:33 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 81F015A5F77 Received: by smtp36.i.mail.ru with esmtpa (envelope-from ) id 1qe944-00BHzg-0s; Thu, 07 Sep 2023 10:04:32 +0300 Message-ID: Date: Thu, 7 Sep 2023 10:04:31 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.0 To: Sergey Kaplun , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org, max.kokryashkin@gmail.com References: Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD96E142CFC92DB15CD9A78593717343123184EC05241EE6705182A05F5380850401259BF85EFE1EBBBAC8EDC510D7EEF05F442F1373E7288DC2B9C7DCF4FCAD9EA X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AC4684DF4EC4B256EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006372521E7C1CE72986C8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D83CDE7F682E4BDCCF9ED828321D9E2B34117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCDCBA8CBAA3833548A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD186FD1C55BDD38FC3FD2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE26055571C92BF10F9F804269016115C9D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE32D01283D1ACF37BA6E0066C2D8992A16C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637021B7B85925F8E35EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5219508826505CA970A82B00379C6EFCB1BABD895796BE0FBF87CCE6106E1FC07E67D4AC08A07B9B013BDA61BF53F5E1DCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34829444FF2D8CB89BD1353CFEF4B6FE4EDD4EF9BBA7D7B049DA7B6BBEF04F534B557107ABE6132B571D7E09C32AA3244C1D219B0B1592E555EC4FE055074E8B0EB018FE5BB746DCD1BAD658CF5C8AB4025DA084F8E80FEBD3FFA33E6B6B2F82C47A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojAl6H1x/OglJgLOqPXyuJOA== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A76979864F7204840FD2AC8EDC510D7EEF052F2E01304D987C7FEBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey On 9/5/23 15:55, Sergey Kaplun wrote: > 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. Renamed. > >> 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. Removed. > >> +#include > Ditto. Removed. > >> +#include >> +#include > Ditto. Removed. > >> +#include >> +#include > Ditto. Removed. > >> + >> +#include >> +#include > This include is excess since all libs are opened via utils. Removed. > >> +#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`). Removed. > >> + >> +#include "test.h" >> +#include "utils.h" >> + >> +/* Need for skipcond. */ >> +#include "lj_arch.h" > There is no skipconditions, so this include may be dropped. Removed. > >> + >> +/* 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__. > Fixed. >> + >> +/* Defined in lua.h. */ >> +/* mark for precompiled code (`Lua') */ >> +#define LUA_SIGNATURE "\033Lua" > We already included , so this define isn't required. Fixed. > >> + >> +#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 `/*`. Fixed. > >> + * 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/ Fixed. > > (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. This is exactly the case covered by testcase, note the name of testcase and Reader function contains postfix "_eof". Yes, doesn't check the patch itself, because it is not trivial to test endmark introduced in patch. I added note about test to the second commit too. > >> + { >> + 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. Fixed. > >> + * `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 Fixed. > >> + * 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). Updated. > >> + >> + /** > Typo: s Fixed. > >> + * 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/ Fixed. > >> + */ >> + size_t sz = 2; >> + bc_chunk = malloc(sz); >> + /** > Typo: s Fixed. > >> + * `lua_load` automatically detects whether the chunk is text or binary, > Typo: s/binary,/binary/ Fixed. > >> + * 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. Fixed. > >> + */ >> + 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/=/==/ Fixed. > 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