Re: [Tarantool-patches] [PATCH luajit 1/2][v2] FFI: Treat cdata finalizer table as a GC root.

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

Hi, Sergey!
Thanks for the fixes!
Please consider my minor nits about comments below.

On 09.07.24, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for review. Fixes applied and force-pushed.
> 
> Sergey
> 
> 
> On 09.07.2024 14:52, 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>
> >>
> >> Thanks to Sergey Bronnikov.
> >>
> >> (cherry picked from commit dda1ac273ad946387088d91039a8ae319359903d)
> >>
> >> There is a table `CTState->finalizer` that contains cdata finalizers.
> >> This table is created on initialization of the `ffi` module
> > I suppose we may drop the first sentence and start like the following:
> >
> > | The finalizers table is created...
> 
> Updated.
> 
> 
> >
> >> by calling the functions `luaopen_ffi` and `ffi_finalizer`. In some
> > I suggest the following rewording:
> > | by calling the `ffi_finalizer()` routine in the `luaopen_ffi()`
> 
> Updated.


| The finalizers table is created on initialization of the `ffi`
| module by calling the `ffi_finalizer()` routine in the
| `luaopen_ffi()`.

Here it is good to say that usually `ffi.gc()` is anchored somewhere on
the stack via the ffi library, so the finalizer table is anchored as
well.

|                  But, there is no FFI module table anywhere to

Minor: s/But,/If/ [*]

| anchor the `ffi.gc` itself, and the `lua_State` object was marked

Typo: s/,//

| before the function is placed on it. Hence, after the atomic

[*] s/./, then the finalier table isn't marked./

It is more correct to say, that "`lua_State` is marked after the
function is removed from it" (since we stop the GC before chunk
loading and starts after).

Also, we can say `lua_State` is marked when `ffi.gc()` is not on it.

| phase, the table is considered dead and collected. Since the table
| is collected, the usage of its nodes in the `lj_gc_finalize_cdata`
| leads to heap-use-after-free.

> 
> 
> >
> >> circumstances, this table could be collected by GC and then accessed by
> >> the function `lj_gc_finalize_cdata`. This leads to a heap-use-after-free
> > Please describe more verbosely why this table isn't marked and has
> > become garbage collected. How is it marked before the patch?
> >
> >> problem. The patch fixes the problem.
> > How does the patch fix the problem?
> >
> > Also, it is worth mentioning that the problem was partially solved, the
> > complete fix will be applied in the next patch.

Please, add its description to the commit message too.

> >
> >> Sergey Bronnikov:
> >> * added the description and the tests for the problem
> >>
> >> Part of tarantool/tarantool#10199
> >> ---
> >>   src/lj_gc.c                                   |  3 +
> >>   ...free-on-access-to-CTState-finalizer.test.c | 66 +++++++++++++++++++
> >>   ...ee-on-access-to-CTState-finalizer.test.lua | 18 +++++
> >>   3 files changed, 87 insertions(+)
> >>   create mode 100644 test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
> >>   create mode 100644 test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua

<snipped>

> 
> 
> >
> >> + * has the finalizer table as its environment. But, there is no
> >> + * FFI module table anywhere to anchor the `ffi.gc` itself, and
> >> + * the `lua_State` object was marked before the function is

It is more correct to say, that "`lua_State` is marked after the
function is removed from it" (since we stop the GC before chunk
loading and starts after).

> >> + * placed on it. Hence, after the atomic phase, the table

<snipped>

> 
> >> +{
> >> +	/* Shared Lua state is not needed. */
> >> +	(void)test_state;

<snipped>

> >> +
> >> +	if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK)
> > Why do we need to omit the ending zero byte?

I see no related comment on the branch.

<snipped>

> >> diff --git a/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua b/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua
> >> new file mode 100644
> >> index 00000000..fca5ec76
> >> --- /dev/null
> >> +++ b/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua
> >> @@ -0,0 +1,18 @@

<snipped>

> >
> >> +-- or removing some of the functionality of it and then calls
> >> +-- `collectgarbage`.
> >> +-- Seehttps://github.com/LuaJIT/LuaJIT/issues/1168  for details.
> >> +local test = tap.test('lj-1168-heap-use-after-free-on-access-to-CTState-finalizer')
> > Code line is longer than 80 symbols.
> > Don't to update this testname after renaming of the file.
> Updated.

| +-- This test demonstrates LuaJIT's heap-use-after-free on
| +-- on cleaning of resources during shoutdown. Test simulates

Typo: s/on//
Typo: s/Test/The test/

| +-- "unloading" of the library, or removing some of the

Typo: s/the functionality of it/its functionality/

| +-- functionality of it and then calls `collectgarbage`.
| +-- See https://github.com/LuaJIT/LuaJIT/issues/1168 for details.

> >

<snipped>

> >> -- 
> >> 2.34.1
> >>

-- 
Best regards,
Sergey Kaplun