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 fixes!
LGTM, after fixing minor comments below.

On 10.07.24, Sergey Bronnikov wrote:
> 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>
> >>

<snipped>

> > Minor: "will be collected at the end of the cycle if it is created after
> > the start phase."
> 
> Updated.

| Previous patch fixes the problem partially because the introduced
| 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.

Minor: "cycle (since it isn't marked because it is not accessible from
any GC root)."

|            Access to the cdata finalizer table exhibits heap use
| after free. The patch turns the finalizer table into a proper
> 

<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

<snipped>

> >> +
> >> +	/* 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.

Thanks!

> >
> >> +
> >> +	int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t");

I suggest renaming "chunk" to the "test_chunk" here too.

Also, please add here comment about `sizeof(buff) - 1` too.


> >> +	assert_true(res == LUA_OK);

<snipped>

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

Code line length is more than 80 symbols.
(Same for the previous commit.)


> +               bail_out("error loading Lua chunk");
> +       }
> 
>          /* Finish GC cycle. */
>          while (!lua_gc(L, LUA_GCSTEP, -1));
> 
> >
> >> +
> >> +	/* Finish GC cycle. */

Let's add "to collect the finalizer table." to be consistent with
another test.

> >> +	while (!lua_gc(L, LUA_GCSTEP, -1));
> >> +

<snipped>

-- 
Best regards,
Sergey Kaplun