Hi,

please see comments below. Fixes applied and force-pushed.

Sergey

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