From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: tarantool-patches@dev.tarantool.org, Sergey Kaplun <skaplun@tarantool.org>, Maxim Kokryashkin <m.kokryashkin@tarantool.org> Subject: [Tarantool-patches] [PATCH luajit 1/2][v2] FFI: Treat cdata finalizer table as a GC root. Date: Thu, 15 Aug 2024 11:20:31 +0300 [thread overview] Message-ID: <066c36f1d4a77ffefc020997222d873692bebcc5.1723708977.git.sergeyb@tarantool.org> (raw) In-Reply-To: <cover.1723708977.git.sergeyb@tarantool.org> From: Mike Pall <mike> Thanks to Sergey Bronnikov. (cherry picked from commit dda1ac273ad946387088d91039a8ae319359903d) The finalizers table is created on initialization of the `ffi` module by calling the `ffi_finalizer()` routine in the `luaopen_ffi()`. `ffi.gc()` is referenced by Lua stack via the `ffi` library, and the finalizer table is anchored there as well. If there is no FFI module table anywhere to anchor the `ffi.gc` itself and the `lua_State` object is marked after the function `ffi.gc` is removed from it (since we stop the GC before chunk loading and start after), then the finalizer table isn't marked. 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. The patch fixes the problem partially by marking the finalizer table on the start of the GC cycle. The complete fix will be applied in the next patch by turning the finalizer table into the proper GC root. Sergey Bronnikov: * added the description and the tests for the problem Part of tarantool/tarantool#10199 --- src/lj_gc.c | 3 + .../lj-1168-unmarked-finalizer-tab.test.c | 76 +++++++++++++++++++ .../lj-1168-unmarked-finalizer-tab.test.lua | 18 +++++ 3 files changed, 97 insertions(+) create mode 100644 test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c create mode 100644 test/tarantool-tests/lj-1168-unmarked-finalizer-tab.test.lua 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 @@ -99,6 +99,9 @@ static void gc_mark_start(global_State *g) gc_markobj(g, tabref(mainthread(g)->env)); gc_marktv(g, &g->registrytv); gc_mark_gcroot(g); +#if LJ_HASFFI + if (ctype_ctsG(g)) gc_markobj(g, ctype_ctsG(g)->finalizer); +#endif g->gc.state = GCSpropagate; } diff --git a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c new file mode 100644 index 00000000..d577b551 --- /dev/null +++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c @@ -0,0 +1,76 @@ +#include "lua.h" +#include "lauxlib.h" + +#include "test.h" + +#define UNUSED(x) ((void)(x)) + +/* + * This test demonstrates LuaJIT's incorrect behaviour on + * loading Lua chunk with cdata numbers. + * See https://github.com/LuaJIT/LuaJIT/issues/1168 for details. + * + * The GC is driving forward during parsing of the Lua chunk + * (`test_chunk`). The chunk contains a single cdata object with + * a number. That leads to the opening of the FFI library + * on-demand during the parsing of this number. After the FFI + * library is open, `ffi.gc` 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 is + * marked after the function is removed from 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. + */ + +const char buff[] = "return 1LL"; + +/* + * lua_close is a part of testcase, so testcase creates + * its own Lua state and closes it at the end. + */ +static int unmarked_finalizer_tab_gcstart(void *test_state) +{ + /* Shared Lua state is not needed. */ + UNUSED(test_state); + + /* Setup. */ + lua_State *L = luaL_newstate(); + + /* Set GC at the start. */ + lua_gc(L, LUA_GCCOLLECT, 0); + + /* Not trigger GC during `lua_openffi()`. */ + lua_gc(L, LUA_GCSTOP, 0); + + /* + * The terminating '\0' is considered by parser as part of + * the input, so we must chomp it. + */ + int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, + "test_chunk", "t"); + if (res != LUA_OK) { + test_comment("error loading Lua chunk: %s", + lua_tostring(L, -1)); + bail_out("error loading Lua chunk"); + } + + /* Finish GC cycle to collect the finalizer table. */ + 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-unmarked-finalizer-tab.test.lua b/test/tarantool-tests/lj-1168-unmarked-finalizer-tab.test.lua new file mode 100644 index 00000000..4b49e9a1 --- /dev/null +++ b/test/tarantool-tests/lj-1168-unmarked-finalizer-tab.test.lua @@ -0,0 +1,18 @@ +local tap = require('tap') + +-- This test demonstrates LuaJIT's heap-use-after-free on +-- cleaning of resources during shutdown. The test simulates +-- "unloading" of the library, or removing some of its +-- functionality and then calls `collectgarbage`. +-- See https://github.com/LuaJIT/LuaJIT/issues/1168 for details. +local test = tap.test('lj-1168-unmarked-finalizer-tab') +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
next prev parent reply other threads:[~2024-08-15 8:20 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-08-15 8:15 [Tarantool-patches] [PATCH luajit 0/2][v2] Fix cdata finalizer table Sergey Bronnikov via Tarantool-patches 2024-08-15 8:20 ` Sergey Bronnikov via Tarantool-patches [this message] 2024-08-15 8:59 ` [Tarantool-patches] [PATCH luajit 1/2][v2] FFI: Treat cdata finalizer table as a GC root Maxim Kokryashkin via Tarantool-patches 2024-08-15 8:21 ` [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper " Sergey Bronnikov via Tarantool-patches 2024-08-15 9:38 ` Maxim Kokryashkin via Tarantool-patches 2024-08-15 12:16 ` [Tarantool-patches] [PATCH luajit 0/2][v2] Fix cdata finalizer table Sergey Kaplun via Tarantool-patches -- strict thread matches above, loose matches on Subject: below -- 2024-07-09 10:45 Sergey Bronnikov via Tarantool-patches 2024-07-09 10:45 ` [Tarantool-patches] [PATCH luajit 1/2][v2] FFI: Treat cdata finalizer table as a GC root Sergey Bronnikov via Tarantool-patches 2024-07-09 11:52 ` Sergey Kaplun via Tarantool-patches 2024-07-09 15:43 ` Sergey Bronnikov via Tarantool-patches 2024-07-10 13:13 ` Sergey Kaplun via Tarantool-patches 2024-07-23 18:18 ` Sergey Bronnikov via Tarantool-patches 2024-08-12 13:32 ` Sergey Kaplun via Tarantool-patches 2024-08-15 7:32 ` Sergey Bronnikov via Tarantool-patches 2024-08-15 8:33 ` Sergey Kaplun 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=066c36f1d4a77ffefc020997222d873692bebcc5.1723708977.git.sergeyb@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=estetus@gmail.com \ --cc=m.kokryashkin@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 1/2][v2] FFI: Treat cdata finalizer table as a GC root.' \ /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