[Tarantool-patches] [PATCH luajit 0/2][v2] Fix cdata finalizer table

[Tarantool-patches] [PATCH luajit 0/2][v2] Fix cdata finalizer table

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer-nointegration
NOTE: Jobs with Tarantool regression tests has failed because
patch "FFI: Turn FFI finalizer table into a proper GC root."
broke Tarantool build and fix (see below) must be applied before
a bump to LuaJIT version with proposed patches.

Branch with fix in Tarantool: https://github.com/ligurio/tarantool/tree/ligurio/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer
Issues:
- https://github.com/luaJIT/luaJIT/issues/1168
- https://github.com/tarantool/tarantool/issues/10199

Mike Pall (2):
  FFI: Treat cdata finalizer table as a GC root.
  FFI: Turn FFI finalizer table into a proper GC root.

 src/lib_ffi.c                                 |  20 +---
 src/lj_cdata.c                                |   2 +-
 src/lj_ctype.c                                |  12 ++
 src/lj_ctype.h                                |   2 +-
 src/lj_gc.c                                   |  38 +++---
 src/lj_obj.h                                  |   3 +
 src/lj_state.c                                |   3 +
 ...free-on-access-to-CTState-finalizer.test.c | 108 ++++++++++++++++++
 ...ee-on-access-to-CTState-finalizer.test.lua |  18 +++
 9 files changed, 165 insertions(+), 41 deletions(-)
 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

-- 
2.34.1

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

[Sergey Bronnikov via Tarantool-patches]

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
by calling the functions `luaopen_ffi` and `ffi_finalizer`. In some
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
problem. The patch fixes the problem.

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

diff --git a/src/lj_gc.c b/src/lj_gc.c
index 591862b3..42348a34 100644
--- a/src/lj_gc.c
+++ b/src/lj_gc.c
@@ -99,6 +99,9 @@ static void gc_mark_start(global_State *g)
   gc_markobj(g, tabref(mainthread(g)->env));
   gc_marktv(g, &g->registrytv);
   gc_mark_gcroot(g);
+#if LJ_HASFFI
+  if (ctype_ctsG(g)) gc_markobj(g, ctype_ctsG(g)->finalizer);
+#endif
   g->gc.state = GCSpropagate;
 }
 
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
new file mode 100644
index 00000000..c388c6a7
--- /dev/null
+++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
@@ -0,0 +1,66 @@
+#include <string.h>
+
+#include "lua.h"
+#include "lauxlib.h"
+
+#include "test.h"
+
+/*
+ * This test demonstrates LuaJIT's incorrect behaviour on
+ * loading Lua chunk.
+ * See https://github.com/LuaJIT/LuaJIT/issues/1168 for details.
+ *
+ * The GC is driving forward during parsing of the Lua chunk (`chunk`).
+ * The chunk contains plenty of global objects, and the parsing
+ * of this creates a lot of strings to be used as keys in `_G`.
+ * Also, it contains several imaginary numbers (1i, 4i, 8i).
+ * That leads to the opening of the FFI library on-demand during the
+ * parsing of these numbers. 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 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.
+ */
+
+const char buff[] = "return 1LL";
+
+/*
+ * lua_close is a part of testcase, so testcase creates
+ * it's own Lua state and closes it at the end.
+ */
+static int unmarked_finalizer_tab_gcstart(void *test_state)
+{
+	/* 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);
+
+	if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK)
+		printf("Err: %s\n", lua_tostring(L, -1));
+
+	/* Finish GC cycle. */
+	while (!lua_gc(L, LUA_GCSTEP, -1));
+
+	/* 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),
+	};
+	const int test_result = test_run_group(tgroup, NULL);
+
+	return test_result;
+}
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 @@
+local tap = require('tap')
+
+-- This test demonstrates LuaJIT's heap-use-after-free
+-- on collecting garbage. Test simulates "unloading" of the library,
+-- or removing some of the functionality of it and then calls
+-- `collectgarbage`.
+-- See https://github.com/LuaJIT/LuaJIT/issues/1168 for details.
+local test = tap.test('lj-1168-heap-use-after-free-on-access-to-CTState-finalizer')
+test:plan(1)
+
+local ffi = require('ffi')
+
+ffi.gc = nil
+collectgarbage()
+
+test:ok(true, 'no heap use after free')
+
+test:done(true)
-- 
2.34.1

[Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

[Sergey Bronnikov via Tarantool-patches]

From: Mike Pall <mike>

Reported by Sergey Bronnikov.

(cherry picked from commit f5affaa6c4e7524e661484f22f24255f9a83eb47)

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. Access to the cdata finalizer table exhibits heap use
after free. The patch is turned the finalizer table into a proper
GC root.

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
@@ -513,7 +513,7 @@ LJLIB_CF(ffi_new)	LJLIB_REC(.)
     /* Handle ctype __gc metamethod. Use the fast lookup here. */
     cTValue *tv = lj_tab_getinth(cts->miscmap, -(int32_t)id);
     if (tv && tvistab(tv) && (tv = lj_meta_fast(L, tabV(tv), MM_gc))) {
-      GCtab *t = cts->finalizer;
+      GCtab *t = tabref(G(L)->gcroot[GCROOT_FFI_FIN]);
       if (gcref(t->metatable)) {
 	/* Add to finalizer table, if still enabled. */
 	copyTV(L, lj_tab_set(L, t, o-1), tv);
@@ -762,7 +762,7 @@ LJLIB_CF(ffi_abi)	LJLIB_REC(.)
   return 1;
 }
 
-LJLIB_PUSH(top-8) LJLIB_SET(!)  /* Store reference to miscmap table. */
+LJLIB_PUSH(top-7) LJLIB_SET(!)  /* Store reference to miscmap table. */
 
 LJLIB_CF(ffi_metatype)
 {
@@ -788,8 +788,6 @@ LJLIB_CF(ffi_metatype)
   return 1;
 }
 
-LJLIB_PUSH(top-7) LJLIB_SET(!)  /* Store reference to finalizer table. */
-
 LJLIB_CF(ffi_gc)	LJLIB_REC(.)
 {
   GCcdata *cd = ffi_checkcdata(L, 1);
@@ -822,19 +820,6 @@ LJLIB_PUSH(top-2) LJLIB_SET(arch)
 
 /* ------------------------------------------------------------------------ */
 
-/* Create special weak-keyed finalizer table. */
-static GCtab *ffi_finalizer(lua_State *L)
-{
-  /* NOBARRIER: The table is new (marked white). */
-  GCtab *t = lj_tab_new(L, 0, 1);
-  settabV(L, L->top++, t);
-  setgcref(t->metatable, obj2gco(t));
-  setstrV(L, lj_tab_setstr(L, t, lj_str_newlit(L, "__mode")),
-	  lj_str_newlit(L, "k"));
-  t->nomm = (uint8_t)(~(1u<<MM_mode));
-  return t;
-}
-
 /* Register FFI module as loaded. */
 static void ffi_register_module(lua_State *L)
 {
@@ -850,7 +835,6 @@ LUALIB_API int luaopen_ffi(lua_State *L)
 {
   CTState *cts = lj_ctype_init(L);
   settabV(L, L->top++, (cts->miscmap = lj_tab_new(L, 0, 1)));
-  cts->finalizer = ffi_finalizer(L);
   LJ_LIB_REG(L, NULL, ffi_meta);
   /* NOBARRIER: basemt is a GC root. */
   setgcref(basemt_it(G(L), LJ_TCDATA), obj2gco(tabV(L->top-1)));
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
@@ -89,7 +89,7 @@ void LJ_FASTCALL lj_cdata_free(global_State *g, GCcdata *cd)
 
 void lj_cdata_setfin(lua_State *L, GCcdata *cd, GCobj *obj, uint32_t it)
 {
-  GCtab *t = ctype_ctsG(G(L))->finalizer;
+  GCtab *t = tabref(G(L)->gcroot[GCROOT_FFI_FIN]);
   if (gcref(t->metatable)) {
     /* Add cdata to finalizer table, if still enabled. */
     TValue *tv, tmp;
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
@@ -643,6 +643,18 @@ CTState *lj_ctype_init(lua_State *L)
   return cts;
 }
 
+/* Create special weak-keyed finalizer table. */
+void lj_ctype_initfin(lua_State *L)
+{
+  /* NOBARRIER: The table is new (marked white). */
+  GCtab *t = lj_tab_new(L, 0, 1);
+  setgcref(t->metatable, obj2gco(t));
+  setstrV(L, lj_tab_setstr(L, t, lj_str_newlit(L, "__mode")),
+	  lj_str_newlit(L, "k"));
+  t->nomm = (uint8_t)(~(1u<<MM_mode));
+  setgcref(G(L)->gcroot[GCROOT_FFI_FIN], obj2gco(t));
+}
+
 /* Free C type table and state. */
 void lj_ctype_freestate(global_State *g)
 {
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
@@ -177,7 +177,6 @@ typedef struct CTState {
   MSize sizetab;	/* Size of C type table. */
   lua_State *L;		/* Lua state (needed for errors and allocations). */
   global_State *g;	/* Global state. */
-  GCtab *finalizer;	/* Map of cdata to finalizer. */
   GCtab *miscmap;	/* Map of -CTypeID to metatable and cb slot to func. */
   CCallback cb;		/* Temporary callback state. */
   CTypeID1 hash[CTHASH_SIZE];  /* Hash anchors for C type table. */
@@ -473,6 +472,7 @@ LJ_FUNC GCstr *lj_ctype_repr(lua_State *L, CTypeID id, GCstr *name);
 LJ_FUNC GCstr *lj_ctype_repr_int64(lua_State *L, uint64_t n, int isunsigned);
 LJ_FUNC GCstr *lj_ctype_repr_complex(lua_State *L, void *sp, CTSize size);
 LJ_FUNC CTState *lj_ctype_init(lua_State *L);
+LJ_FUNC void lj_ctype_initfin(lua_State *L);
 LJ_FUNC void lj_ctype_freestate(global_State *g);
 
 #endif
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
@@ -99,9 +99,6 @@ static void gc_mark_start(global_State *g)
   gc_markobj(g, tabref(mainthread(g)->env));
   gc_marktv(g, &g->registrytv);
   gc_mark_gcroot(g);
-#if LJ_HASFFI
-  if (ctype_ctsG(g)) gc_markobj(g, ctype_ctsG(g)->finalizer);
-#endif
   g->gc.state = GCSpropagate;
 }
 
@@ -181,8 +178,7 @@ static int gc_traverse_tab(global_State *g, GCtab *t)
     }
     if (weak) {  /* Weak tables are cleared in the atomic phase. */
 #if LJ_HASFFI
-      CTState *cts = ctype_ctsG(g);
-      if (cts && cts->finalizer == t) {
+      if (gcref(g->gcroot[GCROOT_FFI_FIN]) == obj2gco(t)) {
 	weak = (int)(~0u & ~LJ_GC_WEAKVAL);
       } else
 #endif
@@ -550,7 +546,7 @@ static void gc_finalize(lua_State *L)
     o->gch.marked &= (uint8_t)~LJ_GC_CDATA_FIN;
     /* Resolve finalizer. */
     setcdataV(L, &tmp, gco2cd(o));
-    tv = lj_tab_set(L, ctype_ctsG(g)->finalizer, &tmp);
+    tv = lj_tab_set(L, tabref(g->gcroot[GCROOT_FFI_FIN]), &tmp);
     if (!tvisnil(tv)) {
       g->gc.nocdatafin = 0;
       copyTV(L, &tmp, tv);
@@ -582,23 +578,20 @@ void lj_gc_finalize_udata(lua_State *L)
 void lj_gc_finalize_cdata(lua_State *L)
 {
   global_State *g = G(L);
-  CTState *cts = ctype_ctsG(g);
-  if (cts) {
-    GCtab *t = cts->finalizer;
-    Node *node = noderef(t->node);
-    ptrdiff_t i;
-    setgcrefnull(t->metatable);  /* Mark finalizer table as disabled. */
-    for (i = (ptrdiff_t)t->hmask; i >= 0; i--)
-      if (!tvisnil(&node[i].val) && tviscdata(&node[i].key)) {
-	GCobj *o = gcV(&node[i].key);
-	TValue tmp;
-	makewhite(g, o);
-	o->gch.marked &= (uint8_t)~LJ_GC_CDATA_FIN;
-	copyTV(L, &tmp, &node[i].val);
-	setnilV(&node[i].val);
-	gc_call_finalizer(g, L, &tmp, o);
-      }
-  }
+  GCtab *t = tabref(g->gcroot[GCROOT_FFI_FIN]);
+  Node *node = noderef(t->node);
+  ptrdiff_t i;
+  setgcrefnull(t->metatable);  /* Mark finalizer table as disabled. */
+  for (i = (ptrdiff_t)t->hmask; i >= 0; i--)
+    if (!tvisnil(&node[i].val) && tviscdata(&node[i].key)) {
+      GCobj *o = gcV(&node[i].key);
+      TValue tmp;
+      makewhite(g, o);
+      o->gch.marked &= (uint8_t)~LJ_GC_CDATA_FIN;
+      copyTV(L, &tmp, &node[i].val);
+      setnilV(&node[i].val);
+      gc_call_finalizer(g, L, &tmp, o);
+    }
 }
 #endif
 
@@ -721,7 +714,7 @@ static size_t gc_onestep(lua_State *L)
       return GCFINALIZECOST;
     }
 #if LJ_HASFFI
-    if (!g->gc.nocdatafin) lj_tab_rehash(L, ctype_ctsG(g)->finalizer);
+    if (!g->gc.nocdatafin) lj_tab_rehash(L, tabref(g->gcroot[GCROOT_FFI_FIN]));
 #endif
     g->gc.state = GCSpause;  /* End of GC cycle. */
     g->gc.debt = 0;
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
@@ -580,6 +580,9 @@ typedef enum {
   GCROOT_BASEMT_NUM = GCROOT_BASEMT + ~LJ_TNUMX,
   GCROOT_IO_INPUT,	/* Userdata for default I/O input file. */
   GCROOT_IO_OUTPUT,	/* Userdata for default I/O output file. */
+#if LJ_HASFFI
+  GCROOT_FFI_FIN,	/* FFI finalizer table. */
+#endif
   GCROOT_MAX
 } GCRootID;
 
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
@@ -180,6 +180,9 @@ static TValue *cpluaopen(lua_State *L, lua_CFunction dummy, void *ud)
   lj_lex_init(L);
   fixstring(lj_err_str(L, LJ_ERR_ERRMEM));  /* Preallocate memory error msg. */
   g->gc.threshold = 4*g->gc.total;
+#if LJ_HASFFI
+  lj_ctype_initfin(L);
+#endif
   lj_trace_initstate(g);
   lj_err_verify();
   return NULL;
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";
+
+	/* 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);
+
+	int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t");
+	assert_true(res == LUA_OK);
+
+	/* Finish GC cycle. */
+	while (!lua_gc(L, LUA_GCSTEP, -1));
+
+	assert_true(lua_gettop(L) == 1);
+
+	/* 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

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

[Sergey Kaplun via Tarantool-patches]

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...

> 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()`

> 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.

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

The filenames are too long for my taste. I suggest the following names:
<lj-1168-unmarked-finalizer-tab.test.c>
<lj-1168-unmarked-finalizer-tab.test.lua>

> 
> diff --git a/src/lj_gc.c b/src/lj_gc.c
> index 591862b3..42348a34 100644
> --- a/src/lj_gc.c
> +++ b/src/lj_gc.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
> new file mode 100644
> index 00000000..c388c6a7
> --- /dev/null
> +++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
> @@ -0,0 +1,66 @@
> +#include <string.h>

This include is excess.

> +
> +#include "lua.h"
> +#include "lauxlib.h"
> +
> +#include "test.h"
> +
> +/*
> + * This test demonstrates LuaJIT's incorrect behaviour on
> + * loading Lua chunk.

Minor: chunk with cdata numbers.

> + * See https://github.com/LuaJIT/LuaJIT/issues/1168 for details.
> + *
> + * The GC is driving forward during parsing of the Lua chunk (`chunk`).

Comment line is more than 66 symbols. Here and below.

> + * The chunk contains plenty of global objects, and the parsing
> + * of this creates a lot of strings to be used as keys in `_G`.
> + * Also, it contains several imaginary numbers (1i, 4i, 8i).

This chunk doesn't contains such objects, only cdata number 1LL.

> + * That leads to the opening of the FFI library on-demand during the
> + * parsing of these numbers. After the FFI library is open, `ffi.gc`

(this number).

> + * 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 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.

It will be nice to add this comment to the commit message.

> + */
> +
> +const char buff[] = "return 1LL";
> +
> +/*
> + * lua_close is a part of testcase, so testcase creates
> + * it's own Lua state and closes it at the end.

Typo: s/it's/its/

> + */
> +static int unmarked_finalizer_tab_gcstart(void *test_state)


Minor: Maybe rename this variable like the following:
test_state -> unused;

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

Maybe it is better to introduce the UNUSED macro in the <utils.h> for
it. We already defined such macro for the following tests:

* fix-yield-c-hook.test.c
* lj-549-lua-load.test.c
* misclib-sysprof-capi.test.c
* unit-tap.test.c

> +
> +	/* Setup. */
> +	lua_State *L = luaL_newstate();
> +
> +	/* Set GC at the start. */
> +	lua_gc(L, LUA_GCCOLLECT, 0);
> +
> +	if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK)

Why do we need to omit the ending zero byte?

Maybe name "test_chunk" is better? Feel free to ignore.

> +		printf("Err: %s\n", lua_tostring(L, -1));

Please use bail_out instead of `printf()`.

| test_comment("error running payload: %s", lua_tostring(L, -1));
| bail_out("error running " __func__ " test payload");

Maybe it is worth introducing some helpers in the <utils.h> for loading
and running Lua code inside our C tests.

> +
> +	/* Finish GC cycle. */

Minor: s/./ to collect the finalizer table./

> +	while (!lua_gc(L, LUA_GCSTEP, -1));
> +
> +	/* 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),
> +	};
> +	const int test_result = test_run_group(tgroup, NULL);
> +
> +	return test_result;
> +}
> 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 @@
> +local tap = require('tap')
> +
> +-- This test demonstrates LuaJIT's heap-use-after-free
> +-- on collecting garbage. Test simulates "unloading" of the library,

I'd rather say that "on cleaning of resources during shoutdown", since
it happens during `lua_close()`.

Comment width is more than 66 lines.

> +-- or removing some of the functionality of it and then calls
> +-- `collectgarbage`.
> +-- See https://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.

> +test:plan(1)
> +
> +local ffi = require('ffi')
> +
> +ffi.gc = nil
> +collectgarbage()
> +
> +test:ok(true, 'no heap use after free')
> +
> +test:done(true)
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 0/2][v2] Fix cdata finalizer table

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patchset!

On 09.07.24, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer-nointegration
> NOTE: Jobs with Tarantool regression tests has failed because
> patch "FFI: Turn FFI finalizer table into a proper GC root."
> broke Tarantool build and fix (see below) must be applied before
> a bump to LuaJIT version with proposed patches.
> 
> Branch with fix in Tarantool: https://github.com/ligurio/tarantool/tree/ligurio/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer

May you please create a PR to run all tests?

> Issues:
> - https://github.com/luaJIT/luaJIT/issues/1168
> - https://github.com/tarantool/tarantool/issues/10199

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

[Sergey Kaplun via Tarantool-patches]

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/

> 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."

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

Typo: s/is turned/turns/

> 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.

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

Why do we need this assert?

> +
> +	/* 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
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

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.


>
>> 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.
>
>> 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
> The filenames are too long for my taste. I suggest the following names:
> <lj-1168-unmarked-finalizer-tab.test.c>
> <lj-1168-unmarked-finalizer-tab.test.lua>
Renamed.
>
>> diff --git a/src/lj_gc.c b/src/lj_gc.c
>> index 591862b3..42348a34 100644
>> --- a/src/lj_gc.c
>> +++ b/src/lj_gc.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
>> new file mode 100644
>> index 00000000..c388c6a7
>> --- /dev/null
>> +++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
>> @@ -0,0 +1,66 @@
>> +#include <string.h>
> This include is excess.

Removed.


>
>> +
>> +#include "lua.h"
>> +#include "lauxlib.h"
>> +
>> +#include "test.h"
>> +
>> +/*
>> + * This test demonstrates LuaJIT's incorrect behaviour on
>> + * loading Lua chunk.
> Minor: chunk with cdata numbers.

Updated.


>
>> + * Seehttps://github.com/LuaJIT/LuaJIT/issues/1168  for details.
>> + *
>> + * The GC is driving forward during parsing of the Lua chunk (`chunk`).
> Comment line is more than 66 symbols. Here and below.

Updated.


>
>> + * The chunk contains plenty of global objects, and the parsing
>> + * of this creates a lot of strings to be used as keys in `_G`.
>> + * Also, it contains several imaginary numbers (1i, 4i, 8i).
> This chunk doesn't contains such objects, only cdata number 1LL.
Right, updated.
>
>> + * That leads to the opening of the FFI library on-demand during the
>> + * parsing of these numbers. After the FFI library is open, `ffi.gc`
> (this number).

Fixed.


>
>> + * 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 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.
> It will be nice to add this comment to the commit message.
Added.
>
>> + */
>> +
>> +const char buff[] = "return 1LL";
>> +
>> +/*
>> + * lua_close is a part of testcase, so testcase creates
>> + * it's own Lua state and closes it at the end.
> Typo: s/it's/its/

Fixed.


>
>> + */
>> +static int unmarked_finalizer_tab_gcstart(void *test_state)
>
> Minor: Maybe rename this variable like the following:
> test_state -> unused;

Renamed.


>> +{
>> +	/* Shared Lua state is not needed. */
>> +	(void)test_state;
> Maybe it is better to introduce the UNUSED macro in the <utils.h> for
> it. We already defined such macro for the following tests:
>
> * fix-yield-c-hook.test.c
> * lj-549-lua-load.test.c
> * misclib-sysprof-capi.test.c
> * unit-tap.test.c

Added a macro UNUSED and renamed unused to test_data back,

because `UNUSED(unused)` doesn't look very good (because масло масляное).

>
>> +
>> +	/* Setup. */
>> +	lua_State *L = luaL_newstate();
>> +
>> +	/* Set GC at the start. */
>> +	lua_gc(L, LUA_GCCOLLECT, 0);
>> +
>> +	if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK)
> Why do we need to omit the ending zero byte?
>
> Maybe name "test_chunk" is better? Feel free to ignore.
Renamed.
>
>> +		printf("Err: %s\n", lua_tostring(L, -1));
> Please use bail_out instead of `printf()`.

Fixed.


>
> | test_comment("error running payload: %s", lua_tostring(L, -1));
> | bail_out("error running " __func__ " test payload");
>
> Maybe it is worth introducing some helpers in the <utils.h> for loading
> and running Lua code inside our C tests.

I don't think this code is complicated enough to introduce a helper for it.

Ignored.

>
>> +
>> +	/* Finish GC cycle. */
> Minor: s/./ to collect the finalizer table./

Updated.


>
>> +	while (!lua_gc(L, LUA_GCSTEP, -1));
>> +
>> +	/* 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),
>> +	};
>> +	const int test_result = test_run_group(tgroup, NULL);
>> +
>> +	return test_result;
>> +}
>> 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 @@
>> +local tap = require('tap')
>> +
>> +-- This test demonstrates LuaJIT's heap-use-after-free
>> +-- on collecting garbage. Test simulates "unloading" of the library,
> I'd rather say that "on cleaning of resources during shoutdown", since
> it happens during `lua_close()`.
Updated.
>
> Comment width is more than 66 lines.
>
>> +-- 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.
>
>> +test:plan(1)
>> +
>> +local ffi = require('ffi')
>> +
>> +ffi.gc = nil
>> +collectgarbage()
>> +
>> +test:ok(true, 'no heap use after free')
>> +
>> +test:done(true)
>> -- 
>> 2.34.1
>>

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

Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

[Sergey Bronnikov via Tarantool-patches]

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

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>
>>
>> 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.
Added.
>> 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?
Left a single buffer.
>
>> +
>> +	/* 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.
Ok, I'll add.
>
>> +
>> +	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
>>

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

Re: [Tarantool-patches] [PATCH luajit 0/2][v2] Fix cdata finalizer table

[Sergey Bronnikov via Tarantool-patches]

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


On 09.07.2024 14:54, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patchset!
>
> On 09.07.24, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov<sergeyb@tarantool.org>
>>
>> Branch:https://github.com/tarantool/luajit/tree/ligurio/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer-nointegration
>> NOTE: Jobs with Tarantool regression tests has failed because
>> patch "FFI: Turn FFI finalizer table into a proper GC root."
>> broke Tarantool build and fix (see below) must be applied before
>> a bump to LuaJIT version with proposed patches.
>>
>> Branch with fix in Tarantool:https://github.com/ligurio/tarantool/tree/ligurio/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer
> May you please create a PR to run all tests?

Sure, it was created before -

https://github.com/tarantool/tarantool/pull/9796


>
>> Issues:
>> -https://github.com/luaJIT/luaJIT/issues/1168
>> -https://github.com/tarantool/tarantool/issues/10199
> <snipped>
>

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

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

[Sergey Kaplun via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

[Sergey Kaplun via Tarantool-patches]

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.

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.

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

<snipped>

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

[-- 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 --]

Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

fixes applied and force-pushed.

Sergey

On 10.07.2024 17:08, Sergey Kaplun wrote:
> 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)."
Updated.
>
> |            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.)
Fixed for both commits.
>
>
>> +               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>
>

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

Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!

On 23.07.24, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> fixes applied and force-pushed.
> 
> Sergey
> 
> On 10.07.2024 17:08, Sergey Kaplun wrote:
> > 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:

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

> >>>> +	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?

OK, let's leave it as "chunk" in both places.

> 
> 
> >
> > Also, please add here comment about `sizeof(buff) - 1` too.

Please add a comment that the terminating '\0' is considered by parser
as part of the input, so we must chomp it.

> >

<snipped>

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

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/


> >

<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.

> >
> >>>> 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/

> >

<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.

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

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

[-- 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 --]

Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

[Sergey Bronnikov via Tarantool-patches]

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

Hello, Sergey,


On 12.08.2024 16:17, Sergey Kaplun wrote:


<snipped>

>>
>>> Also, please add here comment about `sizeof(buff) - 1` too.
> Please add a comment that the terminating '\0' is considered by parser
> as part of the input, so we must chomp it.

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
@@ -89,6 +89,10 @@ unmarked_finalizer_tab_gcmark(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>
>

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

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

[Sergey Kaplun via Tarantool-patches]

Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

[Sergey Kaplun via Tarantool-patches]

Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 0/2][v2] Fix cdata finalizer table

[Sergey Kaplun via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.1 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/10423
[2]: https://github.com/tarantool/tarantool/pull/10424
[3]: https://github.com/tarantool/tarantool/pull/10425

-- 
Best regards,
Sergey Kaplun

[Tarantool-patches] [PATCH luajit 0/2][v2] Fix cdata finalizer table

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Branch:
https://github.com/tarantool/luajit/tree/ligurio/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer-nointegration
PR in Tarantool: https://github.com/tarantool/tarantool/pull/9796

NOTE: Jobs with Tarantool integration regression tests has failed
because patch "FFI: Turn FFI finalizer table into a proper GC root."
broke Tarantool build and fix (see below) must be applied before
a bump to LuaJIT version with proposed patches.

Mike Pall (2):
  FFI: Treat cdata finalizer table as a GC root.
  FFI: Turn FFI finalizer table into a proper GC root.

 src/lib_ffi.c                                 |  20 +--
 src/lj_cdata.c                                |   2 +-
 src/lj_ctype.c                                |  12 ++
 src/lj_ctype.h                                |   2 +-
 src/lj_gc.c                                   |  38 +++---
 src/lj_obj.h                                  |   3 +
 src/lj_state.c                                |   3 +
 .../lj-1168-unmarked-finalizer-tab.test.c     | 123 ++++++++++++++++++
 .../lj-1168-unmarked-finalizer-tab.test.lua   |  18 +++
 9 files changed, 180 insertions(+), 41 deletions(-)
 create mode 100644 test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
 create mode 100644 test/tarantool-tests/lj-1168-unmarked-finalizer-tab.test.lua

-- 
2.34.1