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: 4668 bytes --]

Hello, Sergey,

thanks for review! See my answers below.

On 12.08.2024 16:32, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
>
> On 23.07.24, Sergey Bronnikov wrote:
>> 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>
>>>>>>
> <snipped>
>
>>> 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).
> My bad:
> Typo: s/starts/start/

Fixed.


>
>
> <snipped>
>
>>>>> 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.
>
> I would rephrase this part as the following:
>
> | The patch fixes the problem partially by marking the finalizer table
> | on the start of the GC cycle.
> | The complete fix will be applied in the next patch by turning the
> | finalizer table into the proper GC root.
Updated, thanks!
>>>>>> 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).
> My bad:
> Typo: s/starts/start/

Updated:

--- a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
+++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
@@ -16,8 +16,8 @@
   * on-demand during the parsing of this number. After the FFI
   * library is open, `ffi.gc` 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 placed on it. Hence, after the
+ * anchor the `ffi.gc` itself, and the `lua_State` object is
+ * marked after the function is removed from it. Hence, after the
   * atomic 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.


> <snipped>
>
>>>>>> +	if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK)
>>>>> Why do we need to omit the ending zero byte?
> Please add a comment that the terminating '\0' is considered by parser
> as part of the input, so we must chomp it.
>
Added:


@@ -43,6 +43,10 @@ static int unmarked_finalizer_tab_gcstart(void 
*test_state)
         /* Not trigger GC during `lua_openffi()`. */
         lua_gc(L, LUA_GCSTOP, 0);

+       /*
+        * The terminating '\0' is considered by parser as part of
+        * the input, so we must chomp it.
+        */
         int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,
                                    "test_chunk", "t");
         if (res != LUA_OK) {

>>> <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
> <snipped>
>
>>>>>> 2.34.1
>>>>>>

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