Hi, Sergey

thanks for review. Fixes applied and force-pushed.

Sergey

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 09.07.24, Sergey Bronnikov wrote:

From: Mike Pall <mike>

Reported by Sergey Bronnikov.

(cherry picked from commit f5affaa6c4e7524e661484f22f24255f9a83eb47)

Previous patch fixes the problem partially because the introduced

Typo: s/Previous/The previous/
Typo: s/fixes (.*) partially/partially fixes \1/

Fixed.

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. Access to the cdata finalizer table exhibits heap use

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

Updated.

after free. The patch is turned the finalizer table into a proper

Typo: s/is turned/turns/

Updated.

GC root.

It is worth mentioning that this table is created on the initialization
of the main Lua State instead of loading the FFI library.

Sergey Bronnikov:
* added the description and the tests for the problem

Part of tarantool/tarantool#10199
---
 src/lib_ffi.c                                 | 20 +--------
 src/lj_cdata.c                                |  2 +-
 src/lj_ctype.c                                | 12 ++++++
 src/lj_ctype.h                                |  2 +-
 src/lj_gc.c                                   | 41 ++++++++----------
 src/lj_obj.h                                  |  3 ++
 src/lj_state.c                                |  3 ++
 ...free-on-access-to-CTState-finalizer.test.c | 42 +++++++++++++++++++
 8 files changed, 81 insertions(+), 44 deletions(-)

diff --git a/src/lib_ffi.c b/src/lib_ffi.c
index 7ed6fc78..3c8dd77f 100644
--- a/src/lib_ffi.c
+++ b/src/lib_ffi.c

<snipped>

diff --git a/src/lj_cdata.c b/src/lj_cdata.c
index 35d0e76a..3d6ff1cc 100644
--- a/src/lj_cdata.c
+++ b/src/lj_cdata.c

<snipped>

diff --git a/src/lj_ctype.c b/src/lj_ctype.c
index 53b83031..c0213629 100644
--- a/src/lj_ctype.c
+++ b/src/lj_ctype.c

<snipped>

diff --git a/src/lj_ctype.h b/src/lj_ctype.h
index 8edbd561..2d393eb9 100644
--- a/src/lj_ctype.h
+++ b/src/lj_ctype.h

<snipped>

diff --git a/src/lj_gc.c b/src/lj_gc.c
index 42348a34..4c222f21 100644
--- a/src/lj_gc.c
+++ b/src/lj_gc.c

<snipped>

diff --git a/src/lj_obj.h b/src/lj_obj.h
index 69e94ff2..06ea0cd0 100644
--- a/src/lj_obj.h
+++ b/src/lj_obj.h

<snipped>

diff --git a/src/lj_state.c b/src/lj_state.c
index 01d4901a..5a920102 100644
--- a/src/lj_state.c
+++ b/src/lj_state.c

<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
@@ -55,10 +55,52 @@ static int unmarked_finalizer_tab_gcstart(void *test_state)
 	return TEST_EXIT_SUCCESS;
 }
 
+static int
+unmarked_finalizer_tab_gcsweep(void *test_state)
+{
+	const char buff[] = "return 1LL";

Why do we need an additional buffer if the same one already exists?

+
+	/* Shared Lua state is not needed. */
+	(void)test_state;
+
+	/* Setup. */
+	lua_State *L = luaL_newstate();
+
+	/* Set GC at the start. */
+	lua_gc(L, LUA_GCCOLLECT, 0);
+
+	/*
+	 * Default step is too big -- one step ends after the
+	 * atomic phase.
+	 */
+	lua_gc(L, LUA_GCSETSTEPMUL, 1);
+
+	/* Skip marking roots. */
+	lua_gc(L, LUA_GCSTEP, 1);
+
+	/* 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.

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

I suppose it is better to use not assert_true here but `test_comment()`
and `bail_out()`, since this is not behaviour that we are testing.

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
@@ -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));
+               bail_out("error loading Lua chunk");
+       }
 
        /* Finish GC cycle. */
        while (!lua_gc(L, LUA_GCSTEP, -1));

+
+	/* Finish GC cycle. */
+	while (!lua_gc(L, LUA_GCSTEP, -1));
+
+	assert_true(lua_gettop(L) == 1);

Why do we need this assert?

removed

+
+	/* Teardown. */
+	lua_settop(L, 0);
+	lua_close(L);
+
+	return TEST_EXIT_SUCCESS;
+}
+
 int main(void)
 {
 	const struct test_unit tgroup[] = {
 		test_unit_def(unmarked_finalizer_tab_gcstart),
+		test_unit_def(unmarked_finalizer_tab_gcsweep),
 	};
 	const int test_result = test_run_group(tgroup, NULL);
 
-- 
2.34.1