Hello, Sergey,

thanks for review! See my answers below.

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.

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