Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper 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>
> 
> 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