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

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

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

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

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

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

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

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