[Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.
Sergey Kaplun
skaplun at tarantool.org
Tue Jul 9 15:14:39 MSK 2024
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/
> 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."
> after free. The patch is turned the finalizer table into a proper
Typo: s/is turned/turns/
> 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.
>
> 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?
> +
> + /* 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.
> +
> + 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.
> +
> + /* Finish GC cycle. */
> + while (!lua_gc(L, LUA_GCSTEP, -1));
> +
> + assert_true(lua_gettop(L) == 1);
Why do we need this assert?
> +
> + /* 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
>
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list