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

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

[-- Attachment #1: Type: text/plain, Size: 5436 bytes --]

Hi,

please see comments below. Fixes applied and force-pushed.

Sergey

On 10.07.2024 16:13, Sergey Kaplun wrote:
> 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/ [*]
Fixed.
>
> | anchor the `ffi.gc` itself, and the `lua_State` object was marked
>
> Typo: s/,//
Fixed.
>
> | 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.
>
Updated.
>>
>>>> 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.
Added.
> 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/
Fixed. And "shoutdown" as well was fixed.
>
> | +-- "unloading" of the library, or removing some of the
>
> Typo: s/the functionality of it/its functionality/
Fixed.
>
> | +-- functionality of it and then calls `collectgarbage`.
> | +-- Seehttps://github.com/LuaJIT/LuaJIT/issues/1168  for details.
>
> <snipped>
>
>>>> -- 
>>>> 2.34.1
>>>>

[-- Attachment #2: Type: text/html, Size: 10605 bytes --]