Hi, Sergey, thanks for review. Fixes applied and force-pushed. Sergey On 09.07.2024 14:52, Sergey Kaplun via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the patch! > Please consider my comments below. > > On 09.07.24, Sergey Bronnikov wrote: >> From: Mike Pall >> >> Thanks to Sergey Bronnikov. >> >> (cherry picked from commit dda1ac273ad946387088d91039a8ae319359903d) >> >> There is a table `CTState->finalizer` that contains cdata finalizers. >> This table is created on initialization of the `ffi` module > I suppose we may drop the first sentence and start like the following: > > | The finalizers table is created... Updated. > >> by calling the functions `luaopen_ffi` and `ffi_finalizer`. In some > I suggest the following rewording: > | by calling the `ffi_finalizer()` routine in the `luaopen_ffi()` Updated. > >> circumstances, this table could be collected by GC and then accessed by >> the function `lj_gc_finalize_cdata`. This leads to a heap-use-after-free > Please describe more verbosely why this table isn't marked and has > become garbage collected. How is it marked before the patch? > >> problem. The patch fixes the problem. > How does the patch fix the problem? > Also, it is worth mentioning that the problem was partially solved, the > complete fix will be applied in the next patch. > >> Sergey Bronnikov: >> * added the description and the tests for the problem >> >> Part of tarantool/tarantool#10199 >> --- >> src/lj_gc.c | 3 + >> ...free-on-access-to-CTState-finalizer.test.c | 66 +++++++++++++++++++ >> ...ee-on-access-to-CTState-finalizer.test.lua | 18 +++++ >> 3 files changed, 87 insertions(+) >> create mode 100644 test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c >> create mode 100644 test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua > The filenames are too long for my taste. I suggest the following names: > > Renamed. > >> diff --git a/src/lj_gc.c b/src/lj_gc.c >> index 591862b3..42348a34 100644 >> --- a/src/lj_gc.c >> +++ b/src/lj_gc.c > > >> diff --git a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c >> new file mode 100644 >> index 00000000..c388c6a7 >> --- /dev/null >> +++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c >> @@ -0,0 +1,66 @@ >> +#include > This include is excess. Removed. > >> + >> +#include "lua.h" >> +#include "lauxlib.h" >> + >> +#include "test.h" >> + >> +/* >> + * This test demonstrates LuaJIT's incorrect behaviour on >> + * loading Lua chunk. > Minor: chunk with cdata numbers. Updated. > >> + * Seehttps://github.com/LuaJIT/LuaJIT/issues/1168 for details. >> + * >> + * The GC is driving forward during parsing of the Lua chunk (`chunk`). > Comment line is more than 66 symbols. Here and below. Updated. > >> + * The chunk contains plenty of global objects, and the parsing >> + * of this creates a lot of strings to be used as keys in `_G`. >> + * Also, it contains several imaginary numbers (1i, 4i, 8i). > This chunk doesn't contains such objects, only cdata number 1LL. Right, updated. > >> + * That leads to the opening of the FFI library on-demand during the >> + * parsing of these numbers. After the FFI library is open, `ffi.gc` > (this number). Fixed. > >> + * has the finalizer table as its environment. But, there is no >> + * FFI module table anywhere to anchor the `ffi.gc` itself, and >> + * the `lua_State` object was marked before the function is >> + * placed on it. Hence, after the atomic phase, the table >> + * is considered dead and collected. Since the table is collected, >> + * the usage of its nodes in the `lj_gc_finalize_cdata` leads >> + * to heap-use-after-free. > It will be nice to add this comment to the commit message. Added. > >> + */ >> + >> +const char buff[] = "return 1LL"; >> + >> +/* >> + * lua_close is a part of testcase, so testcase creates >> + * it's own Lua state and closes it at the end. > Typo: s/it's/its/ Fixed. > >> + */ >> +static int unmarked_finalizer_tab_gcstart(void *test_state) > > Minor: Maybe rename this variable like the following: > test_state -> unused; Renamed. >> +{ >> + /* Shared Lua state is not needed. */ >> + (void)test_state; > Maybe it is better to introduce the UNUSED macro in the for > it. We already defined such macro for the following tests: > > * fix-yield-c-hook.test.c > * lj-549-lua-load.test.c > * misclib-sysprof-capi.test.c > * unit-tap.test.c Added a macro UNUSED and renamed unused to test_data back, because `UNUSED(unused)` doesn't look very good (because масло масляное). > >> + >> + /* Setup. */ >> + lua_State *L = luaL_newstate(); >> + >> + /* Set GC at the start. */ >> + lua_gc(L, LUA_GCCOLLECT, 0); >> + >> + if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK) > Why do we need to omit the ending zero byte? > > Maybe name "test_chunk" is better? Feel free to ignore. Renamed. > >> + printf("Err: %s\n", lua_tostring(L, -1)); > Please use bail_out instead of `printf()`. Fixed. > > | test_comment("error running payload: %s", lua_tostring(L, -1)); > | bail_out("error running " __func__ " test payload"); > > Maybe it is worth introducing some helpers in the for loading > and running Lua code inside our C tests. I don't think this code is complicated enough to introduce a helper for it. Ignored. > >> + >> + /* Finish GC cycle. */ > Minor: s/./ to collect the finalizer table./ Updated. > >> + while (!lua_gc(L, LUA_GCSTEP, -1)); >> + >> + /* Teardown. */ >> + lua_settop(L, 0); >> + lua_close(L); >> + >> + return TEST_EXIT_SUCCESS; >> +} >> + >> +int main(void) >> +{ >> + const struct test_unit tgroup[] = { >> + test_unit_def(unmarked_finalizer_tab_gcstart), >> + }; >> + const int test_result = test_run_group(tgroup, NULL); >> + >> + return test_result; >> +} >> diff --git a/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua b/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua >> new file mode 100644 >> index 00000000..fca5ec76 >> --- /dev/null >> +++ b/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua >> @@ -0,0 +1,18 @@ >> +local tap = require('tap') >> + >> +-- This test demonstrates LuaJIT's heap-use-after-free >> +-- on collecting garbage. Test simulates "unloading" of the library, > I'd rather say that "on cleaning of resources during shoutdown", since > it happens during `lua_close()`. Updated. > > Comment width is more than 66 lines. > >> +-- or removing some of the functionality of it and then calls >> +-- `collectgarbage`. >> +-- Seehttps://github.com/LuaJIT/LuaJIT/issues/1168 for details. >> +local test = tap.test('lj-1168-heap-use-after-free-on-access-to-CTState-finalizer') > Code line is longer than 80 symbols. > Don't to update this testname after renaming of the file. Updated. > >> +test:plan(1) >> + >> +local ffi = require('ffi') >> + >> +ffi.gc = nil >> +collectgarbage() >> + >> +test:ok(true, 'no heap use after free') >> + >> +test:done(true) >> -- >> 2.34.1 >>