From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org>, Sergey Bronnikov <estetus@gmail.com> Cc: tarantool-patches@dev.tarantool.org, max.kokryashkin@gmail.com Subject: Re: [Tarantool-patches] [PATCH luajit 2/2][v2] Followup fix for embedded bytecode loader. Date: Thu, 7 Sep 2023 10:04:31 +0300 [thread overview] Message-ID: <ed1c9205-0761-6713-9cd3-04c682111262@tarantool.org> (raw) In-Reply-To: <ZPclWaf_x9YbrKF9@root> 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 <sergeyb@tarantool.org> >> >> (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 <assert.h> > This include is excess. Removed. > >> +#include <stdint.h> > Ditto. Removed. > >> +#include <stddef.h> >> +#include <string.h> > Ditto. Removed. > >> +#include <stdlib.h> >> +#include <stdio.h> > Ditto. Removed. > >> + >> +#include <lua.h> >> +#include <lualib.h> > This include is excess since all libs are opened via utils. Removed. > >> +#include <lauxlib.h> > 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 (`<esc>Lua') */ >> +#define LUA_SIGNATURE "\033Lua" > We already included <lua.h>, 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: > > | <src/lj_lex.c:50> > | 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
next prev parent reply other threads:[~2023-09-07 7:04 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-08-31 11:29 [Tarantool-patches] [PATCH luajit 0/2][v2] Fix " Sergey Bronnikov via Tarantool-patches 2023-08-31 11:30 ` [Tarantool-patches] [PATCH luajit 1/2][v2] " Sergey Bronnikov via Tarantool-patches 2023-08-31 11:49 ` Sergey Bronnikov via Tarantool-patches 2023-09-01 9:42 ` Maxim Kokryashkin via Tarantool-patches 2023-09-04 9:31 ` Sergey Bronnikov via Tarantool-patches 2023-09-05 6:34 ` Maxim Kokryashkin via Tarantool-patches 2023-09-05 14:10 ` Sergey Kaplun via Tarantool-patches 2023-09-07 15:21 ` Sergey Bronnikov via Tarantool-patches 2023-09-11 8:45 ` Sergey Kaplun via Tarantool-patches 2023-09-12 10:20 ` Sergey Bronnikov via Tarantool-patches 2023-10-31 11:30 ` Sergey Kaplun via Tarantool-patches 2023-09-05 14:12 ` Sergey Kaplun via Tarantool-patches 2023-09-07 7:06 ` Sergey Bronnikov via Tarantool-patches 2023-08-31 11:32 ` [Tarantool-patches] [PATCH luajit 2/2][v2] Followup fix for " Sergey Bronnikov via Tarantool-patches 2023-09-01 10:05 ` Maxim Kokryashkin via Tarantool-patches 2023-09-04 16:34 ` Sergey Bronnikov via Tarantool-patches 2023-09-05 6:45 ` Maxim Kokryashkin via Tarantool-patches 2023-09-05 12:55 ` Sergey Kaplun via Tarantool-patches 2023-09-07 7:04 ` Sergey Bronnikov via Tarantool-patches [this message] 2023-09-11 9:26 ` Sergey Kaplun via Tarantool-patches 2023-09-12 10:30 ` Sergey Bronnikov via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ed1c9205-0761-6713-9cd3-04c682111262@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=estetus@gmail.com \ --cc=max.kokryashkin@gmail.com \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 2/2][v2] Followup fix for embedded bytecode loader.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox