Hi, Sergey,

fixes applied and force-pushed.

Sergey

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.

Fixed, but after this the line becomes longer max length and I need to split it for two lines:

--- a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
+++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
@@ -83,7 +83,8 @@ unmarked_finalizer_tab_gcmark(void *test_state)
        /* Not trigger GC during `lua_openffi()`. */
        lua_gc(L, LUA_GCSTOP, 0);
 
-       int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t");
+       int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,
+                                  "test_chunk", "t");
        if (res != LUA_OK) {
                test_comment("error loading Lua chunk: %s", lua_tostring(L, -1));
                bail_out("error loading Lua chunk");

I would leave "chunk" due to this. And you?

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.

Fixed:

--- a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
+++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
@@ -93,7 +93,7 @@ unmarked_finalizer_tab_gcmark(void *test_state)
                bail_out("error loading Lua chunk");
        }
 
-       /* Finish GC cycle. */
+       /* Finish GC cycle to collect the finalizer table. */
        while (!lua_gc(L, LUA_GCSTEP, -1));
 
        /* Teardown. */

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

<snipped>