Re: [Tarantool-patches] [PATCH luajit] FFI: Avoid dangling cts->L.

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5179 bytes --]

Hi, Sergey,

thanks for the patch!

The bug is not reproduced without patch.

CMake configuration: cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug

Sergey

On 2/25/26 14:57, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by ZumiKua.
>
> (cherry picked from commit c94312d348e3530b369b4e517fce4c65df6cd270)
>
> When executing cdata finalizer on some lua_State, this state is set to
> `cts->L`. If the state will be GC-ed and freed later, this reference
> becomes dangling, so any FFI callback will use this invalid reference.
>
> This patch fixes it by setting `cts->L` to the `mainthread` on the
> destruction of the referenced one.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#12134
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1405-dangling-cts-L
> Related issues:
> *https://github.com/LuaJIT/LuaJIT/issues/1405
> *https://github.com/tarantool/tarantool/issues/12134
>
>   src/lj_state.c                                |  4 ++
>   test/tarantool-c-tests/CMakeLists.txt         |  2 +
>   .../lj-1405-dangling-cts-L.test.c             | 72 +++++++++++++++++++
>   3 files changed, 78 insertions(+)
>   create mode 100644 test/tarantool-c-tests/lj-1405-dangling-cts-L.test.c
>
> diff --git a/src/lj_state.c b/src/lj_state.c
> index 053e5ec9..2eec5857 100644
> --- a/src/lj_state.c
> +++ b/src/lj_state.c
> @@ -360,6 +360,10 @@ void LJ_FASTCALL lj_state_free(global_State *g, lua_State *L)
>     lj_assertG(L != mainthread(g), "free of main thread");
>     if (obj2gco(L) == gcref(g->cur_L))
>       setgcrefnull(g->cur_L);
> +#if LJ_HASFFI
> +  if (ctype_ctsG(g) && ctype_ctsG(g)->L == L)  /* Avoid dangling cts->L. */
> +    ctype_ctsG(g)->L = mainthread(g);
> +#endif
>     lj_func_closeuv(L, tvref(L->stack));
>     lj_assertG(gcref(L->openupval) == NULL, "stale open upvalues");
>     lj_mem_freevec(g, tvref(L->stack), L->stacksize, TValue);
> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> index 32a8add0..3bb20bff 100644
> --- a/test/tarantool-c-tests/CMakeLists.txt
> +++ b/test/tarantool-c-tests/CMakeLists.txt
> @@ -59,6 +59,8 @@ foreach(test_source ${tests})
>       OUTPUT_NAME "${exe}${C_TEST_SUFFIX}"
>       RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
>     )
> +  # Allow to call non-static functions via FFI.
> +  target_link_options(${exe} PRIVATE "-rdynamic")
>     target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY})
>     add_dependencies(tarantool-c-tests-build ${exe})
>   
> diff --git a/test/tarantool-c-tests/lj-1405-dangling-cts-L.test.c b/test/tarantool-c-tests/lj-1405-dangling-cts-L.test.c
> new file mode 100644
> index 00000000..b4ed4970
> --- /dev/null
> +++ b/test/tarantool-c-tests/lj-1405-dangling-cts-L.test.c
> @@ -0,0 +1,72 @@
> +#include "lua.h"
> +
> +#include "test.h"
> +#include "utils.h"
> +
> +/* XXX: Still need normal assert inside `call_callback()`. */
> +#undef NDEBUG
> +#include <assert.h>
> +
> +typedef void (*callback_t)(void);
> +static callback_t callback = NULL;
> +
> +/* Function to be called via FFI. */
> +extern void add_callback(callback_t cb)
> +{
> +	callback = cb;
> +}
> +
> +static void call_callback(void)
> +{
> +	assert(callback != NULL);
> +	callback();
> +}
> +
> +static int dangling_cts_L(void *test_state)
> +{
> +	lua_State *L = utils_lua_init();
> +	luaopen_ffi(L);
> +	const char code[] = {
> +		" local ffi = require('ffi')                               \n" \
> +		" ffi.cdef [[                                              \n" \
> +		"   struct test { int a; };                                \n" \
> +		"   void add_callback(void (*cb)(void a));                 \n" \
> +		    /* Simple finalizer, nop. */
> +		"   int getpid(void);                                      \n" \
> +		" ]]                                                       \n" \
> +		"                                                          \n" \
> +		" local C = ffi.C                                          \n" \
> +		"                                                          \n" \
> +		" local function nop() end                                 \n" \
> +		/* Collected later. Set `cts->L` in the finalizer. */
> +		" ffi.gc(ffi.new('struct test'), C.getpid);                \n" \
> +		/* Callback to be called on the old `cts->L`. */
> +		" C.add_callback(ffi.cast('void (*)(void)', nop))          \n"
> +	};
> +	if (luaL_dostring(L, code) != LUA_OK) {
> +		test_comment("error running Lua chunk: %s",
> +			     lua_tostring(L, -1));
> +		bail_out("error running Lua chunk");
> +	}
> +	lua_State* newL = lua_newthread(L);
> +	/* Remove `newL` from `L`. */
> +	lua_pop(L, 1);
> +	/* Set `cts->L = newL` in the finalizer. */
> +	lua_gc(newL, LUA_GCCOLLECT, 0);
> +	/* Just to be sure we don't use it anymore. */
> +	newL = NULL;
> +	/* Collect `newL`. */
> +	lua_gc(L, LUA_GCCOLLECT, 0);
> +	/* Use after free before the patch. */
> +	call_callback();
> +	utils_lua_close(L);
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +int main(void)
> +{
> +	const struct test_unit tgroup[] = {
> +		test_unit_def(dangling_cts_L),
> +	};
> +	return test_run_group(tgroup, NULL);
> +}

[-- Attachment #2: Type: text/html, Size: 5748 bytes --]