Re: [Tarantool-patches] [PATCH luajit 1/2][v2] FFI: Treat cdata finalizer table as a GC root.

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

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 09.07.24, Sergey Bronnikov wrote:
> From: Mike Pall <mike>
> 
> 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...

> 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()`

> 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:
<lj-1168-unmarked-finalizer-tab.test.c>
<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

<snipped>

> 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 <string.h>

This include is excess.

> +
> +#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.

> + * See https://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.

> + * 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.

> + * 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).

> + * 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.

> + */
> +
> +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/

> + */
> +static int unmarked_finalizer_tab_gcstart(void *test_state)


Minor: Maybe rename this variable like the following:
test_state -> unused;

> +{
> +	/* Shared Lua state is not needed. */
> +	(void)test_state;

Maybe it is better to introduce the UNUSED macro in the <utils.h> 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

> +
> +	/* 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.

> +		printf("Err: %s\n", lua_tostring(L, -1));

Please use bail_out instead of `printf()`.

| 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 <utils.h> for loading
and running Lua code inside our C tests.

> +
> +	/* Finish GC cycle. */

Minor: s/./ 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-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()`.

Comment width is more than 66 lines.

> +-- or removing some of the functionality of it and then calls
> +-- `collectgarbage`.
> +-- See https://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.

> +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
> 

-- 
Best regards,
Sergey Kaplun