[Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.
Sergey Bronnikov
sergeyb at tarantool.org
Wed Jul 10 14:39:53 MSK 2024
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
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20240710/73be579e/attachment.htm>
More information about the Tarantool-patches
mailing list