Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

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

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

Hi, Sergey

thanks for review. Fixes applied and force-pushed.

Sergey


On 09.07.2024 15:14, 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>
>>
>> Reported by Sergey Bronnikov.
>>
>> (cherry picked from commit f5affaa6c4e7524e661484f22f24255f9a83eb47)
>>
>> Previous patch fixes the problem partially because the introduced
> Typo: s/Previous/The previous/
> Typo: s/fixes (.*) partially/partially fixes \1/

Fixed.


>
>> GC root may not exist at the start phase of the GC cycle. In that
>> case, the cdata finalizer table will be collected at the end of
>> the cycle. Access to the cdata finalizer table exhibits heap use
> Minor: "will be collected at the end of the cycle if it is created after
> the start phase."

Updated.


>
>> after free. The patch is turned the finalizer table into a proper
> Typo: s/is turned/turns/

Updated.


>> GC root.
> It is worth mentioning that this table is created on the initialization
> of the main Lua State instead of loading the FFI library.
Added.
>> Sergey Bronnikov:
>> * added the description and the tests for the problem
>>
>> Part of tarantool/tarantool#10199
>> ---
>>   src/lib_ffi.c                                 | 20 +--------
>>   src/lj_cdata.c                                |  2 +-
>>   src/lj_ctype.c                                | 12 ++++++
>>   src/lj_ctype.h                                |  2 +-
>>   src/lj_gc.c                                   | 41 ++++++++----------
>>   src/lj_obj.h                                  |  3 ++
>>   src/lj_state.c                                |  3 ++
>>   ...free-on-access-to-CTState-finalizer.test.c | 42 +++++++++++++++++++
>>   8 files changed, 81 insertions(+), 44 deletions(-)
>>
>> diff --git a/src/lib_ffi.c b/src/lib_ffi.c
>> index 7ed6fc78..3c8dd77f 100644
>> --- a/src/lib_ffi.c
>> +++ b/src/lib_ffi.c
> <snipped>
>
>> diff --git a/src/lj_cdata.c b/src/lj_cdata.c
>> index 35d0e76a..3d6ff1cc 100644
>> --- a/src/lj_cdata.c
>> +++ b/src/lj_cdata.c
> <snipped>
>
>> diff --git a/src/lj_ctype.c b/src/lj_ctype.c
>> index 53b83031..c0213629 100644
>> --- a/src/lj_ctype.c
>> +++ b/src/lj_ctype.c
> <snipped>
>
>> diff --git a/src/lj_ctype.h b/src/lj_ctype.h
>> index 8edbd561..2d393eb9 100644
>> --- a/src/lj_ctype.h
>> +++ b/src/lj_ctype.h
> <snipped>
>
>> diff --git a/src/lj_gc.c b/src/lj_gc.c
>> index 42348a34..4c222f21 100644
>> --- a/src/lj_gc.c
>> +++ b/src/lj_gc.c
> <snipped>
>
>> diff --git a/src/lj_obj.h b/src/lj_obj.h
>> index 69e94ff2..06ea0cd0 100644
>> --- a/src/lj_obj.h
>> +++ b/src/lj_obj.h
> <snipped>
>
>> diff --git a/src/lj_state.c b/src/lj_state.c
>> index 01d4901a..5a920102 100644
>> --- a/src/lj_state.c
>> +++ b/src/lj_state.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
>> index c388c6a7..259528cb 100644
>> --- 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
>> @@ -55,10 +55,52 @@ static int unmarked_finalizer_tab_gcstart(void *test_state)
>>   	return TEST_EXIT_SUCCESS;
>>   }
>>   
>> +static int
>> +unmarked_finalizer_tab_gcsweep(void *test_state)
>> +{
>> +	const char buff[] = "return 1LL";
> Why do we need an additional buffer if the same one already exists?
Left a single buffer.
>
>> +
>> +	/* Shared Lua state is not needed. */
>> +	(void)test_state;
>> +
>> +	/* Setup. */
>> +	lua_State *L = luaL_newstate();
>> +
>> +	/* Set GC at the start. */
>> +	lua_gc(L, LUA_GCCOLLECT, 0);
>> +
>> +	/*
>> +	 * Default step is too big -- one step ends after the
>> +	 * atomic phase.
>> +	 */
>> +	lua_gc(L, LUA_GCSETSTEPMUL, 1);
>> +
>> +	/* Skip marking roots. */
>> +	lua_gc(L, LUA_GCSTEP, 1);
>> +
>> +	/* Not trigger GC during `lua_openffi()`. */
>> +	lua_gc(L, LUA_GCSTOP, 0);
> Maybe it is worth adding this GC stop for the first test case too to
> make it more robust.
Ok, I'll add.
>
>> +
>> +	int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t");
>> +	assert_true(res == LUA_OK);
> I suppose it is better to use not assert_true here but `test_comment()`
> and `bail_out()`, since this is not behaviour that we are testing.

Updated:


--- a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
+++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
@@ -78,7 +78,10 @@ unmarked_finalizer_tab_gcsweep(void *test_state)
         lua_gc(L, LUA_GCSTOP, 0);

         int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", 
"t");
-       assert_true(res == LUA_OK);
+       if (res != LUA_OK) {
+               test_comment("error loading Lua chunk: %s", 
lua_tostring(L, -1));
+               bail_out("error loading Lua chunk");
+       }

         /* Finish GC cycle. */
         while (!lua_gc(L, LUA_GCSTEP, -1));

>
>> +
>> +	/* Finish GC cycle. */
>> +	while (!lua_gc(L, LUA_GCSTEP, -1));
>> +
>> +	assert_true(lua_gettop(L) == 1);
> Why do we need this assert?

removed


>
>> +
>> +	/* 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),
>> +		test_unit_def(unmarked_finalizer_tab_gcsweep),
>>   	};
>>   	const int test_result = test_run_group(tgroup, NULL);
>>   
>> -- 
>> 2.34.1
>>

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