[Tarantool-patches] [PATCH luajit 0/6] FFI fixes

[Tarantool-patches] [PATCH luajit 0/6] FFI fixes

[Sergey Kaplun via Tarantool-patches]

This patchset is an umbrella for all FFI fixes and refactoring to be
done in #9145.

The first patch is a refactoring, and it will be useful for us when we
do #4738. The next two patches are just some FFI bugs to be fixed. The
last three patches are minor fixes use-after-free references of the
`cts->tab` objects.

Branch: https://github.com/tarantool/luajit/tree/skaplun/ffi-fixes
Tarantool PR: https://github.com/tarantool/tarantool/pull/9285
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/745
* https://github.com/LuaJIT/LuaJIT/issues/920
* https://github.com/LuaJIT/LuaJIT/issues/1108
* https://github.com/tarantool/tarantool/issues/9145
* https://github.com/tarantool/tarantool/issues/4738

Mike Pall (6):
  Abstract out on-demand loading of FFI library.
  FFI: Fix missing cts->L initialization in argv2ctype().
  FFI: Ensure returned string is alive in ffi.typeinfo().
  FFI: Fix dangling reference to CType.
  FFI: Fix dangling reference to CType. Improve checks.
  FFI: Fix dangling reference to CType in carith_checkarg().

 src/lib_ffi.c                                 |  1 +
 src/lib_jit.c                                 |  6 +-
 src/lj_bcread.c                               |  6 +-
 src/lj_carith.c                               |  4 +
 src/lj_cconv.c                                |  2 +
 src/lj_crecord.c                              |  6 +-
 src/lj_ctype.c                                | 12 +++
 src/lj_ctype.h                                | 10 +++
 src/lj_lex.c                                  |  6 +-
 .../fix-argv2ctype-cts-L-init.test.lua        | 15 ++++
 .../fix-argv2ctype-cts-L-init/script.lua      | 14 ++++
 .../fix-dangling-reference-to-ctype.test.lua  | 59 ++++++++++++++
 ...8-fix-dangling-reference-to-ctype.test.lua | 67 ++++++++++++++++
 .../lj-745-ffi-typeinfo-dead-names.test.lua   | 28 +++++++
 ...0-fix-dangling-reference-to-ctype.test.lua | 77 +++++++++++++++++++
 15 files changed, 297 insertions(+), 16 deletions(-)
 create mode 100644 test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua
 create mode 100644 test/tarantool-tests/fix-argv2ctype-cts-L-init/script.lua
 create mode 100644 test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
 create mode 100644 test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
 create mode 100644 test/tarantool-tests/lj-745-ffi-typeinfo-dead-names.test.lua
 create mode 100644 test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua

-- 
2.42.0

[Tarantool-patches] [PATCH luajit 1/6] Abstract out on-demand loading of FFI library.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

(cherry-picked from commit 50d6883e6027c4c2f9a5e495fee6b7fff1bd73c9)

This patch introduces the macro `ctype_loadffi(L)` to avoid code
duplication while loading the FFI library.

Sergey Kaplun:
* added the description

Part of tarantool/tarantool#9145
---
 src/lib_jit.c   |  6 +-----
 src/lj_bcread.c |  6 +-----
 src/lj_ctype.h  | 10 ++++++++++
 src/lj_lex.c    |  6 +-----
 4 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/src/lib_jit.c b/src/lib_jit.c
index b3c1c93c..f705f334 100644
--- a/src/lib_jit.c
+++ b/src/lib_jit.c
@@ -339,11 +339,7 @@ LJLIB_CF(jit_util_tracek)
       ir = &T->ir[ir->op1];
     }
 #if LJ_HASFFI
-    if (ir->o == IR_KINT64 && !ctype_ctsG(G(L))) {
-      ptrdiff_t oldtop = savestack(L, L->top);
-      luaopen_ffi(L);  /* Load FFI library on-demand. */
-      L->top = restorestack(L, oldtop);
-    }
+    if (ir->o == IR_KINT64) ctype_loadffi(L);
 #endif
     lj_ir_kvalue(L, L->top-2, ir);
     setintV(L->top-1, (int32_t)irt_type(ir->t));
diff --git a/src/lj_bcread.c b/src/lj_bcread.c
index cddf6ff1..4915240f 100644
--- a/src/lj_bcread.c
+++ b/src/lj_bcread.c
@@ -414,11 +414,7 @@ static int bcread_header(LexState *ls)
   if ((flags & BCDUMP_F_FFI)) {
 #if LJ_HASFFI
     lua_State *L = ls->L;
-    if (!ctype_ctsG(G(L))) {
-      ptrdiff_t oldtop = savestack(L, L->top);
-      luaopen_ffi(L);  /* Load FFI library on-demand. */
-      L->top = restorestack(L, oldtop);
-    }
+    ctype_loadffi(L);
 #else
     return 0;
 #endif
diff --git a/src/lj_ctype.h b/src/lj_ctype.h
index c4f3bdde..fce29409 100644
--- a/src/lj_ctype.h
+++ b/src/lj_ctype.h
@@ -389,6 +389,16 @@ static LJ_AINLINE CTState *ctype_cts(lua_State *L)
   return cts;
 }
 
+/* Load FFI library on-demand. */
+#define ctype_loadffi(L) \
+  do { \
+    if (!ctype_ctsG(G(L))) { \
+      ptrdiff_t oldtop = (char *)L->top - mref(L->stack, char); \
+      luaopen_ffi(L); \
+      L->top = (TValue *)(mref(L->stack, char) + oldtop); \
+    } \
+  } while (0)
+
 /* Save and restore state of C type table. */
 #define LJ_CTYPE_SAVE(cts)	CTState savects_ = *(cts)
 #define LJ_CTYPE_RESTORE(cts) \
diff --git a/src/lj_lex.c b/src/lj_lex.c
index cef3c683..7c6319cc 100644
--- a/src/lj_lex.c
+++ b/src/lj_lex.c
@@ -112,11 +112,7 @@ static void lex_number(LexState *ls, TValue *tv)
     GCcdata *cd;
     lj_assertLS(fmt == STRSCAN_I64 || fmt == STRSCAN_U64 || fmt == STRSCAN_IMAG,
 		"unexpected number format %d", fmt);
-    if (!ctype_ctsG(G(L))) {
-      ptrdiff_t oldtop = savestack(L, L->top);
-      luaopen_ffi(L);  /* Load FFI library on-demand. */
-      L->top = restorestack(L, oldtop);
-    }
+    ctype_loadffi(L);
     if (fmt == STRSCAN_IMAG) {
       cd = lj_cdata_new_(L, CTID_COMPLEX_DOUBLE, 2*sizeof(double));
       ((double *)cdataptr(cd))[0] = 0;
-- 
2.42.0

[Tarantool-patches] [PATCH luajit 2/6] FFI: Fix missing cts->L initialization in argv2ctype().

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

(cherry-picked from commit 50d6883e6027c4c2f9a5e495fee6b7fff1bd73c9)

When start trace recording without an initialized `L` in CType State
(possible if the recording is started before any `ffi` library usage),
the corresponding assertion fails in the `lj_ctype_intern()`. This patch
adds missing initialization during recording.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
 src/lj_crecord.c                                  |  2 +-
 .../fix-argv2ctype-cts-L-init.test.lua            | 15 +++++++++++++++
 .../fix-argv2ctype-cts-L-init/script.lua          | 14 ++++++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua
 create mode 100644 test/tarantool-tests/fix-argv2ctype-cts-L-init/script.lua

diff --git a/src/lj_crecord.c b/src/lj_crecord.c
index e1d1110f..10d1dc70 100644
--- a/src/lj_crecord.c
+++ b/src/lj_crecord.c
@@ -78,7 +78,7 @@ static CTypeID argv2ctype(jit_State *J, TRef tr, cTValue *o)
     /* Specialize to the string containing the C type declaration. */
     emitir(IRTG(IR_EQ, IRT_STR), tr, lj_ir_kstr(J, s));
     cp.L = J->L;
-    cp.cts = ctype_ctsG(J2G(J));
+    cp.cts = ctype_cts(J->L);
     oldtop = cp.cts->top;
     cp.srcname = strdata(s);
     cp.p = strdata(s);
diff --git a/test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua b/test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua
new file mode 100644
index 00000000..ee45e424
--- /dev/null
+++ b/test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua
@@ -0,0 +1,15 @@
+local tap = require('tap')
+local test = tap.test('fix-argv2ctype-cts-L-init'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+-- Loading of 'tap' module initialize `cts->L` during parsing.
+-- Run standalone script for testing.
+local script = require('utils').exec.makecmd(arg)
+
+test:plan(1)
+
+local output = script()
+test:is(output, 'OK', 'correct recording with uninitialized cts->L')
+
+test:done(true)
diff --git a/test/tarantool-tests/fix-argv2ctype-cts-L-init/script.lua b/test/tarantool-tests/fix-argv2ctype-cts-L-init/script.lua
new file mode 100644
index 00000000..2131385a
--- /dev/null
+++ b/test/tarantool-tests/fix-argv2ctype-cts-L-init/script.lua
@@ -0,0 +1,14 @@
+local ffi = require('ffi')
+
+jit.opt.start('hotloop=1')
+
+local i = 1
+-- Use `while` to start compilation of the trace at the first
+-- iteration, before `ffi.new()` is called, so `cts->L` is
+-- uninitialized.
+while i < 3 do
+  ffi.new('uint64_t', i)
+  i = i + 1
+end
+
+print('OK')
-- 
2.42.0

[Tarantool-patches] [PATCH luajit 3/6] FFI: Ensure returned string is alive in ffi.typeinfo().

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by zhuizhuhaomeng.

(cherry-picked from commit 94a40bb238092e73f3dc0c3626911a7efa997c22)

`ffi.typeinfo()` doesn't check that the string containing the name type
is alive when it sets this string to the returned table. This leads to
the corresponding assertion failure in `checklivetv()`. This patch flips
the white colour of the stored string to resurrect it.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
 src/lib_ffi.c                                 |  1 +
 .../lj-745-ffi-typeinfo-dead-names.test.lua   | 28 +++++++++++++++++++
 2 files changed, 29 insertions(+)
 create mode 100644 test/tarantool-tests/lj-745-ffi-typeinfo-dead-names.test.lua

diff --git a/src/lib_ffi.c b/src/lib_ffi.c
index 62af54c1..e60e7b19 100644
--- a/src/lib_ffi.c
+++ b/src/lib_ffi.c
@@ -573,6 +573,7 @@ LJLIB_CF(ffi_typeinfo)
       setintV(lj_tab_setstr(L, t, lj_str_newlit(L, "sib")), (int32_t)ct->sib);
     if (gcref(ct->name)) {
       GCstr *s = gco2str(gcref(ct->name));
+      if (isdead(G(L), obj2gco(s))) flipwhite(obj2gco(s));
       setstrV(L, lj_tab_setstr(L, t, lj_str_newlit(L, "name")), s);
     }
     lj_gc_check(L);
diff --git a/test/tarantool-tests/lj-745-ffi-typeinfo-dead-names.test.lua b/test/tarantool-tests/lj-745-ffi-typeinfo-dead-names.test.lua
new file mode 100644
index 00000000..02e13759
--- /dev/null
+++ b/test/tarantool-tests/lj-745-ffi-typeinfo-dead-names.test.lua
@@ -0,0 +1,28 @@
+local tap = require('tap')
+local ffi = require('ffi')
+local test = tap.test('lj-745-ffi-typeinfo-dead-names')
+
+test:plan(1)
+
+-- Start from the beginning of the GC cycle.
+collectgarbage()
+
+local function ctypes_iteration()
+  local i = 1
+  -- Test `checklivetv()` assertion in `setstrV()` inside
+  -- `ffi.typeinfo()`.
+  while ffi.typeinfo(i) do i = i + 1 end
+end
+
+-- Call `ffi.typeinfo()` much enough to be sure that strings with
+-- names of types become dead. The number of iterations is big
+-- enough (more than x2 of the required value) to see assertion
+-- failure guaranteed under Tarantool too (it has much more alive
+-- objects on start).
+for _ = 1, 100 do
+  ctypes_iteration()
+end
+
+test:ok(true, 'no assertion failure inside ffi.typeinfo()')
+
+test:done(true)
-- 
2.42.0

[Tarantool-patches] [PATCH luajit 4/6] FFI: Fix dangling reference to CType.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

(cherry-picked from commit ae533e3a6c009b5df79b11cd5787d249202fa69c)

During the conversion of a cdata function object to some cdata value in
`lj_cconv_ct_tv()`, reallocation of `cts->tab` in `lj_ctype_intern()`
may occur. In that case, the reference to the `CType` object becomes
invalid. This patch saves the `CTypeID` of the given function and gets
its `CType` again after possible reallocation.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
 src/lj_cconv.c                                |  2 +
 .../fix-dangling-reference-to-ctype.test.lua  | 59 +++++++++++++++++++
 2 files changed, 61 insertions(+)
 create mode 100644 test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua

diff --git a/src/lj_cconv.c b/src/lj_cconv.c
index 37c88852..94ca93bb 100644
--- a/src/lj_cconv.c
+++ b/src/lj_cconv.c
@@ -568,7 +568,9 @@ void lj_cconv_ct_tv(CTState *cts, CType *d,
     }
     s = ctype_raw(cts, sid);
     if (ctype_isfunc(s->info)) {
+      CTypeID did = ctype_typeid(cts, d);
       sid = lj_ctype_intern(cts, CTINFO(CT_PTR, CTALIGN_PTR|sid), CTSIZE_PTR);
+      d = ctype_get(cts, did);  /* cts->tab may have been reallocated. */
     } else {
       if (ctype_isenum(s->info)) s = ctype_child(cts, s);
       goto doconv;
diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
new file mode 100644
index 00000000..c0e2c07b
--- /dev/null
+++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
@@ -0,0 +1,59 @@
+local tap = require('tap')
+local ffi = require('ffi')
+local test = tap.test('fix-dangling-reference-to-ctype'):skipcond({
+  -- luacheck: no global
+  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
+})
+
+test:plan(1)
+
+-- This test demonstrates LuaJIT's incorrect behaviour when the
+-- reallocation of `cts->tab` strikes during the conversion of a
+-- TValue (cdata function pointer) to a C type.
+-- The test fails under ASAN.
+
+-- XXX: Just some C functions to be casted. There is no need to
+-- declare their prototypes correctly.
+ffi.cdef[[
+  int malloc(void);
+  int fprintf(void);
+  int printf(void);
+  int memset(void);
+  int memcpy(void);
+  int memmove(void);
+  int getppid(void);
+]]
+
+-- XXX: structure to set `cts->top` to 110.
+local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
+
+-- Anchor table to prevent cdata objects from being collected.
+local anchor = {}
+-- Each call to this function grows `cts->top` by 3.
+local function save_new_func(func)
+  anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
+end
+
+save_new_func(ffi.C.malloc)  -- `cts->top` = 110
+save_new_func(ffi.C.fprintf) -- `cts->top` = 113
+save_new_func(ffi.C.printf)  -- `cts->top` = 116
+save_new_func(ffi.C.memset)  -- `cts->top` = 119
+save_new_func(ffi.C.memcpy)  -- `cts->top` = 122
+
+-- Assertions to check the `cts->top` value and step between
+-- calls.
+assert(ffi.typeinfo(122), 'cts->top >= 122')
+assert(not ffi.typeinfo(123), 'cts->top < 123')
+
+save_new_func(ffi.C.memmove) -- `cts->top` = 125
+
+assert(ffi.typeinfo(125), 'cts->top >= 125')
+assert(not ffi.typeinfo(126), 'cts->top < 126')
+
+-- Last call to grow `cts->top` up to 128, so this causes
+-- `cts->tab` reallocation.
+save_new_func(ffi.C.getppid) -- `cts->top` = 128
+
+test:ok(true, 'no heap-use-after-free in lj_cconv_ct_tv')
+
+test:done(true)
-- 
2.42.0

[Tarantool-patches] [PATCH luajit 5/6] FFI: Fix dangling reference to CType. Improve checks.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by elmknot.

(cherry-picked from commit cc96ab9d513582703f8663a8775a935b56db32b7)

During the recording of an arithmetic operation with a cdata function
object and some cdata value in `recff_cdata_arith()`, reallocation of
`cts->tab` in `lj_ctype_intern()` may occur. In that case, the reference
to the first `CType` object (`s[0]`) becomes invalid. This patch saves
the `CTypeID` of this object and gets its `CType` again after possible
reallocation.

Also, this commit fills `cts->tab` memory with zeros before being freed
when `-DLUAJIT_CTYPE_CHECK_ANCHOR` is defined. It helps to observe
assertion failures in case this memory is used after free.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
 src/lj_crecord.c                              |  4 +
 src/lj_ctype.c                                | 12 +++
 ...0-fix-dangling-reference-to-ctype.test.lua | 77 +++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua

diff --git a/src/lj_crecord.c b/src/lj_crecord.c
index 10d1dc70..e17e512f 100644
--- a/src/lj_crecord.c
+++ b/src/lj_crecord.c
@@ -1502,9 +1502,13 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
 	if (ctype_isenum(ct->info)) ct = ctype_child(cts, ct);
 	goto ok;
       } else if (ctype_isfunc(ct->info)) {
+	CTypeID id0 = i ? ctype_typeid(cts, s[0]) : 0;
 	tr = emitir(IRT(IR_FLOAD, IRT_PTR), tr, IRFL_CDATA_PTR);
 	ct = ctype_get(cts,
 	  lj_ctype_intern(cts, CTINFO(CT_PTR, CTALIGN_PTR|id), CTSIZE_PTR));
+	if (i) {
+	  s[0] = ctype_get(cts, id0);  /* cts->tab may have been reallocated. */
+	}
 	goto ok;
       } else {
 	tr = emitir(IRT(IR_ADD, IRT_PTR), tr, lj_ir_kintp(J, sizeof(GCcdata)));
diff --git a/src/lj_ctype.c b/src/lj_ctype.c
index a42e3d60..0874fa61 100644
--- a/src/lj_ctype.c
+++ b/src/lj_ctype.c
@@ -191,8 +191,20 @@ CTypeID lj_ctype_intern(CTState *cts, CTInfo info, CTSize size)
   }
   id = cts->top;
   if (LJ_UNLIKELY(id >= cts->sizetab)) {
+#ifdef LUAJIT_CTYPE_CHECK_ANCHOR
+    CType *ct;
+#endif
     if (id >= CTID_MAX) lj_err_msg(cts->L, LJ_ERR_TABOV);
+#ifdef LUAJIT_CTYPE_CHECK_ANCHOR
+    ct = lj_mem_newvec(cts->L, id+1, CType);
+    memcpy(ct, cts->tab, id*sizeof(CType));
+    memset(cts->tab, 0, id*sizeof(CType));
+    lj_mem_freevec(cts->g, cts->tab, cts->sizetab, CType);
+    cts->tab = ct;
+    cts->sizetab = id+1;
+#else
     lj_mem_growvec(cts->L, cts->tab, cts->sizetab, CTID_MAX, CType);
+#endif
   }
   cts->top = id+1;
   cts->tab[id].info = info;
diff --git a/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
new file mode 100644
index 00000000..a7e35888
--- /dev/null
+++ b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
@@ -0,0 +1,77 @@
+local tap = require('tap')
+local ffi = require('ffi')
+local test = tap.test('lj-920-fix-dangling-reference-to-ctype'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+  -- luacheck: no global
+  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
+})
+
+test:plan(1)
+
+-- This test demonstrates LuaJIT's incorrect behaviour when the
+-- reallocation of `cts->tab` strikes during the recording of the
+-- cdata metamethod arithmetic.
+-- The test fails under ASAN.
+
+-- XXX: Just some C functions to be casted. There is no need to
+-- declare their prototypes correctly.
+ffi.cdef[[
+  int malloc(void);
+  int fprintf(void);
+  int printf(void);
+  int memset(void);
+  int memcpy(void);
+  int memmove(void);
+  int getppid(void);
+]]
+
+local cfunc_type = ffi.metatype(ffi.typeof('struct {int a;}'), {
+  -- Just some metatable with reloaded arithmetic operator.
+  __add = function(o1, _) return o1 end
+})
+-- Just some cdata with metamethod.
+local test_value = cfunc_type(1)
+
+-- XXX: structure to set `cts->top` to 112.
+local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
+
+-- Anchor table to prevent cdata objects from being collected.
+local anchor = {}
+-- Each call to this function grows `cts->top` by 3.
+local function save_new_func(func)
+  anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
+end
+
+save_new_func(ffi.C.fprintf) -- `cts->top` = 112
+save_new_func(ffi.C.printf)  -- `cts->top` = 115
+save_new_func(ffi.C.memset)  -- `cts->top` = 118
+save_new_func(ffi.C.memcpy)  -- `cts->top` = 121
+save_new_func(ffi.C.malloc)  -- `cts->top` = 124
+
+-- Assertions to check the `cts->top` value and step between
+-- calls.
+assert(ffi.typeinfo(124), 'cts->top >= 124')
+assert(not ffi.typeinfo(125), 'cts->top < 125')
+
+save_new_func(ffi.C.memmove) -- `cts->top` = 127
+
+jit.opt.start('hotloop=1')
+
+-- Just some function to record trace and cause reallocation.
+local function recorded_func(func_arg)
+  local res = test_value + func_arg
+  return res
+end
+recorded_func(ffi.C.malloc)
+
+assert(ffi.typeinfo(127), 'cts->top >= 127')
+assert(not ffi.typeinfo(128), 'cts->top < 128')
+
+-- Last call to grow `cts->top` up to 129, so this causes
+-- `cts->tab` reallocation. Need to use different functions as
+-- an argument.
+recorded_func(ffi.C.getppid)
+
+test:ok(true, 'no heap-use-after-free in recff_cdata_arith')
+
+test:done(true)
-- 
2.42.0

[Tarantool-patches] [PATCH luajit 6/6] FFI: Fix dangling reference to CType in carith_checkarg().

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Sergey Kaplun.

(cherry-picked from commit db944b2b56c86fcf133745976763604d96110285)

During of an arithmetic operation with a cdata function object and some
cdata value in `carith_checkarg()`, reallocation of `cts->tab` in
`lj_ctype_intern()` may occur. In that case, the reference to the first
`CType` object (`ca->ct[0]`) becomes invalid. This patch saves the
`CTypeID` of this object and gets its `CType` again after possible
reallocation.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
 src/lj_carith.c                               |  4 ++
 ...8-fix-dangling-reference-to-ctype.test.lua | 67 +++++++++++++++++++
 2 files changed, 71 insertions(+)
 create mode 100644 test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua

diff --git a/src/lj_carith.c b/src/lj_carith.c
index 4ae1e9ee..4e1d450a 100644
--- a/src/lj_carith.c
+++ b/src/lj_carith.c
@@ -44,9 +44,13 @@ static int carith_checkarg(lua_State *L, CTState *cts, CDArith *ca)
 	p = (uint8_t *)cdata_getptr(p, ct->size);
 	if (ctype_isref(ct->info)) ct = ctype_rawchild(cts, ct);
       } else if (ctype_isfunc(ct->info)) {
+	CTypeID id0 = i ? ctype_typeid(cts, ca->ct[0]) : 0;
 	p = (uint8_t *)*(void **)p;
 	ct = ctype_get(cts,
 	  lj_ctype_intern(cts, CTINFO(CT_PTR, CTALIGN_PTR|id), CTSIZE_PTR));
+	if (i) {  /* cts->tab may have been reallocated. */
+	  ca->ct[0] = ctype_get(cts, id0);
+	}
       }
       if (ctype_isenum(ct->info)) ct = ctype_child(cts, ct);
       ca->ct[i] = ct;
diff --git a/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
new file mode 100644
index 00000000..43a39886
--- /dev/null
+++ b/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
@@ -0,0 +1,67 @@
+local tap = require('tap')
+local ffi = require('ffi')
+local test = tap.test('lj-1108-fix-dangling-reference-to-ctype'):skipcond({
+  -- luacheck: no global
+  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
+})
+
+test:plan(1)
+
+-- This test demonstrates LuaJIT's incorrect behaviour when the
+-- reallocation of `cts->tab` strikes during the arithmetic cdata
+-- metamethod.
+-- The test fails under ASAN.
+
+-- XXX: Just some C functions to be casted. There is no need to
+-- declare their prototypes correctly.
+ffi.cdef[[
+  int malloc(void);
+  int fprintf(void);
+  int printf(void);
+  int memset(void);
+  int memcpy(void);
+  int memmove(void);
+  int getppid(void);
+]]
+
+local cfunc_type = ffi.metatype(ffi.typeof('struct {int a;}'), {
+  -- Just some metatable with reloaded arithmetic operator.
+  __add = function(o1, _) return o1 end
+})
+-- Just some cdata with metamethod.
+local test_value = cfunc_type(1)
+
+-- XXX: structure to set `cts->top` to 112.
+local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
+
+-- Anchor table to prevent cdata objects from being collected.
+local anchor = {}
+-- Each call to this function grows `cts->top` by 3.
+local function save_new_func(func)
+  anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
+end
+
+save_new_func(ffi.C.fprintf) -- `cts->top` = 112
+save_new_func(ffi.C.printf)  -- `cts->top` = 115
+save_new_func(ffi.C.memset)  -- `cts->top` = 118
+save_new_func(ffi.C.memcpy)  -- `cts->top` = 121
+save_new_func(ffi.C.malloc)  -- `cts->top` = 124
+
+-- Assertions to check the `cts->top` value and step between
+-- calls.
+assert(ffi.typeinfo(124), 'cts->top >= 124')
+assert(not ffi.typeinfo(125), 'cts->top < 125')
+
+save_new_func(ffi.C.memmove) -- `cts->top` = 127
+
+assert(ffi.typeinfo(127), 'cts->top >= 127')
+assert(not ffi.typeinfo(128), 'cts->top < 128')
+
+-- Just check cdata arith metamethod. The function argument should
+-- be the second because dangling reference is the CType of the
+-- first argumnent.
+_ = test_value + ffi.C.getppid
+
+test:ok(true, 'no heap-use-after-free in carith_checkarg')
+
+test:done(true)
-- 
2.42.0

Re: [Tarantool-patches] [PATCH luajit 1/6] Abstract out on-demand loading of FFI library.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
 
  
>Понедельник, 23 октября 2023, 12:26 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>From: Mike Pall <mike>
>
>(cherry-picked from commit 50d6883e6027c4c2f9a5e495fee6b7fff1bd73c9)
>
>This patch introduces the macro `ctype_loadffi(L)` to avoid code
>duplication while loading the FFI library.
>
>Sergey Kaplun:
>* added the description
>
>Part of tarantool/tarantool#9145
>---
> src/lib_jit.c | 6 +-----
> src/lj_bcread.c | 6 +-----
> src/lj_ctype.h | 10 ++++++++++
> src/lj_lex.c | 6 +-----
> 4 files changed, 13 insertions(+), 15 deletions(-)
>
>diff --git a/src/lib_jit.c b/src/lib_jit.c
>index b3c1c93c..f705f334 100644
>--- a/src/lib_jit.c
>+++ b/src/lib_jit.c
>@@ -339,11 +339,7 @@ LJLIB_CF(jit_util_tracek)
>       ir = &T->ir[ir->op1];
>     }
> #if LJ_HASFFI
>- if (ir->o == IR_KINT64 && !ctype_ctsG(G(L))) {
>- ptrdiff_t oldtop = savestack(L, L->top);
>- luaopen_ffi(L); /* Load FFI library on-demand. */
>- L->top = restorestack(L, oldtop);
>- }
>+ if (ir->o == IR_KINT64) ctype_loadffi(L);
> #endif
>     lj_ir_kvalue(L, L->top-2, ir);
>     setintV(L->top-1, (int32_t)irt_type(ir->t));
>diff --git a/src/lj_bcread.c b/src/lj_bcread.c
>index cddf6ff1..4915240f 100644
>--- a/src/lj_bcread.c
>+++ b/src/lj_bcread.c
>@@ -414,11 +414,7 @@ static int bcread_header(LexState *ls)
>   if ((flags & BCDUMP_F_FFI)) {
> #if LJ_HASFFI
>     lua_State *L = ls->L;
>- if (!ctype_ctsG(G(L))) {
>- ptrdiff_t oldtop = savestack(L, L->top);
>- luaopen_ffi(L); /* Load FFI library on-demand. */
>- L->top = restorestack(L, oldtop);
>- }
>+ ctype_loadffi(L);
> #else
>     return 0;
> #endif
>diff --git a/src/lj_ctype.h b/src/lj_ctype.h
>index c4f3bdde..fce29409 100644
>--- a/src/lj_ctype.h
>+++ b/src/lj_ctype.h
>@@ -389,6 +389,16 @@ static LJ_AINLINE CTState *ctype_cts(lua_State *L)
>   return cts;
> }
> 
>+/* Load FFI library on-demand. */
>+#define ctype_loadffi(L) \
>+ do { \
>+ if (!ctype_ctsG(G(L))) { \
>+ ptrdiff_t oldtop = (char *)L->top - mref(L->stack, char); \
>+ luaopen_ffi(L); \
>+ L->top = (TValue *)(mref(L->stack, char) + oldtop); \
>+ } \
>+ } while (0)
>+
> /* Save and restore state of C type table. */
> #define LJ_CTYPE_SAVE(cts) CTState savects_ = *(cts)
> #define LJ_CTYPE_RESTORE(cts) \
>diff --git a/src/lj_lex.c b/src/lj_lex.c
>index cef3c683..7c6319cc 100644
>--- a/src/lj_lex.c
>+++ b/src/lj_lex.c
>@@ -112,11 +112,7 @@ static void lex_number(LexState *ls, TValue *tv)
>     GCcdata *cd;
>     lj_assertLS(fmt == STRSCAN_I64 || fmt == STRSCAN_U64 || fmt == STRSCAN_IMAG,
>  "unexpected number format %d", fmt);
>- if (!ctype_ctsG(G(L))) {
>- ptrdiff_t oldtop = savestack(L, L->top);
>- luaopen_ffi(L); /* Load FFI library on-demand. */
>- L->top = restorestack(L, oldtop);
>- }
>+ ctype_loadffi(L);
>     if (fmt == STRSCAN_IMAG) {
>       cd = lj_cdata_new_(L, CTID_COMPLEX_DOUBLE, 2*sizeof(double));
>       ((double *)cdataptr(cd))[0] = 0;
>--
>2.42.0
 

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

Re: [Tarantool-patches] [PATCH luajit 2/6] FFI: Fix missing cts->L initialization in argv2ctype().

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, except for a few nits regarding the commit message.
On Mon, Oct 23, 2023 at 12:22:02PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> (cherry-picked from commit 50d6883e6027c4c2f9a5e495fee6b7fff1bd73c9)
Incorrect hash, should be: a622e2eb559c823d90c7af85935ca63706e4593d
> 
> When start trace recording without an initialized `L` in CType State
> (possible if the recording is started before any `ffi` library usage),
> the corresponding assertion fails in the `lj_ctype_intern()`. This patch
> adds missing initialization during recording.

Please explicitly mention that the test case fails only when the LuaJIT
is built with -DLUA_USE_ASSERT=ON.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#9145
> ---
<snipped>
Best regards,
Maxim Kokryashkin 
 

Re: [Tarantool-patches] [PATCH luajit 3/6] FFI: Ensure returned string is alive in ffi.typeinfo().

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM
 

Re: [Tarantool-patches] [PATCH luajit 4/6] FFI: Fix dangling reference to CType.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On Mon, Oct 23, 2023 at 12:22:04PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> (cherry-picked from commit ae533e3a6c009b5df79b11cd5787d249202fa69c)
> 
> During the conversion of a cdata function object to some cdata value in
> `lj_cconv_ct_tv()`, reallocation of `cts->tab` in `lj_ctype_intern()`
> may occur. In that case, the reference to the `CType` object becomes
> invalid. This patch saves the `CTypeID` of the given function and gets
> its `CType` again after possible reallocation.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#9145
> ---
>  src/lj_cconv.c                                |  2 +
>  .../fix-dangling-reference-to-ctype.test.lua  | 59 +++++++++++++++++++
>  2 files changed, 61 insertions(+)
>  create mode 100644 test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
> 
> diff --git a/src/lj_cconv.c b/src/lj_cconv.c
> index 37c88852..94ca93bb 100644
> --- a/src/lj_cconv.c
> +++ b/src/lj_cconv.c
<snipped>

> diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
> new file mode 100644
> index 00000000..c0e2c07b
> --- /dev/null
> +++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
> @@ -0,0 +1,59 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local test = tap.test('fix-dangling-reference-to-ctype'):skipcond({
> +  -- luacheck: no global
> +  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
> +})
> +
> +test:plan(1)
> +
> +-- This test demonstrates LuaJIT's incorrect behaviour when the
> +-- reallocation of `cts->tab` strikes during the conversion of a
> +-- TValue (cdata function pointer) to a C type.
> +-- The test fails under ASAN.
Let's change the last sentence to 'Before the patch the test fails only
under ASAN' because now it is a bit misleading. 
> +
> +-- XXX: Just some C functions to be casted. There is no need to
> +-- declare their prototypes correctly.
> +ffi.cdef[[
> +  int malloc(void);
> +  int fprintf(void);
> +  int printf(void);
> +  int memset(void);
> +  int memcpy(void);
> +  int memmove(void);
> +  int getppid(void);
> +]]
> +
> +-- XXX: structure to set `cts->top` to 110.
> +local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
> +
> +-- Anchor table to prevent cdata objects from being collected.
> +local anchor = {}
> +-- Each call to this function grows `cts->top` by 3.
Please drop a comment, referring to a point in sources, so the size
of the growth becomes obvious.

> +local function save_new_func(func)
> +  anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
> +end
> +
> +save_new_func(ffi.C.malloc)  -- `cts->top` = 110
> +save_new_func(ffi.C.fprintf) -- `cts->top` = 113
> +save_new_func(ffi.C.printf)  -- `cts->top` = 116
> +save_new_func(ffi.C.memset)  -- `cts->top` = 119
> +save_new_func(ffi.C.memcpy)  -- `cts->top` = 122

Is it possible to bring us to this value of `cts->top`
with a structure?
> +
> +-- Assertions to check the `cts->top` value and step between
> +-- calls.
> +assert(ffi.typeinfo(122), 'cts->top >= 122')
> +assert(not ffi.typeinfo(123), 'cts->top < 123')
> +
> +save_new_func(ffi.C.memmove) -- `cts->top` = 125
> +
> +assert(ffi.typeinfo(125), 'cts->top >= 125')
> +assert(not ffi.typeinfo(126), 'cts->top < 126')
> +
> +-- Last call to grow `cts->top` up to 128, so this causes
> +-- `cts->tab` reallocation.
> +save_new_func(ffi.C.getppid) -- `cts->top` = 128

Should we add an extra assertion after reallocation?
> +
> +test:ok(true, 'no heap-use-after-free in lj_cconv_ct_tv')
> +
> +test:done(true)
> -- 
> 2.42.0
> 

Re: [Tarantool-patches] [PATCH luajit 5/6] FFI: Fix dangling reference to CType. Improve checks.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, after answering the same questions as for the previous patch.
On Mon, Oct 23, 2023 at 12:22:05PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Reported by elmknot.
> 
> (cherry-picked from commit cc96ab9d513582703f8663a8775a935b56db32b7)
> 
> During the recording of an arithmetic operation with a cdata function
> object and some cdata value in `recff_cdata_arith()`, reallocation of
> `cts->tab` in `lj_ctype_intern()` may occur. In that case, the reference
> to the first `CType` object (`s[0]`) becomes invalid. This patch saves
> the `CTypeID` of this object and gets its `CType` again after possible
> reallocation.
> 
> Also, this commit fills `cts->tab` memory with zeros before being freed
> when `-DLUAJIT_CTYPE_CHECK_ANCHOR` is defined. It helps to observe
> assertion failures in case this memory is used after free.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#9145
> ---
>  src/lj_crecord.c                              |  4 +
>  src/lj_ctype.c                                | 12 +++
>  ...0-fix-dangling-reference-to-ctype.test.lua | 77 +++++++++++++++++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> 
> diff --git a/src/lj_crecord.c b/src/lj_crecord.c
> index 10d1dc70..e17e512f 100644
> --- a/src/lj_crecord.c
> +++ b/src/lj_crecord.c
> @@ -1502,9 +1502,13 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
>  	if (ctype_isenum(ct->info)) ct = ctype_child(cts, ct);
>  	goto ok;
>        } else if (ctype_isfunc(ct->info)) {
> +	CTypeID id0 = i ? ctype_typeid(cts, s[0]) : 0;
>  	tr = emitir(IRT(IR_FLOAD, IRT_PTR), tr, IRFL_CDATA_PTR);
>  	ct = ctype_get(cts,
>  	  lj_ctype_intern(cts, CTINFO(CT_PTR, CTALIGN_PTR|id), CTSIZE_PTR));
> +	if (i) {
> +	  s[0] = ctype_get(cts, id0);  /* cts->tab may have been reallocated. */
> +	}
>  	goto ok;
>        } else {
>  	tr = emitir(IRT(IR_ADD, IRT_PTR), tr, lj_ir_kintp(J, sizeof(GCcdata)));
> diff --git a/src/lj_ctype.c b/src/lj_ctype.c
> index a42e3d60..0874fa61 100644
> --- a/src/lj_ctype.c
> +++ b/src/lj_ctype.c
> @@ -191,8 +191,20 @@ CTypeID lj_ctype_intern(CTState *cts, CTInfo info, CTSize size)
>    }
>    id = cts->top;
>    if (LJ_UNLIKELY(id >= cts->sizetab)) {
> +#ifdef LUAJIT_CTYPE_CHECK_ANCHOR
> +    CType *ct;
> +#endif
>      if (id >= CTID_MAX) lj_err_msg(cts->L, LJ_ERR_TABOV);
> +#ifdef LUAJIT_CTYPE_CHECK_ANCHOR
> +    ct = lj_mem_newvec(cts->L, id+1, CType);
> +    memcpy(ct, cts->tab, id*sizeof(CType));
> +    memset(cts->tab, 0, id*sizeof(CType));
> +    lj_mem_freevec(cts->g, cts->tab, cts->sizetab, CType);
> +    cts->tab = ct;
> +    cts->sizetab = id+1;
> +#else
>      lj_mem_growvec(cts->L, cts->tab, cts->sizetab, CTID_MAX, CType);
> +#endif
>    }
>    cts->top = id+1;
>    cts->tab[id].info = info;
> diff --git a/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> new file mode 100644
> index 00000000..a7e35888
> --- /dev/null
> +++ b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> @@ -0,0 +1,77 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local test = tap.test('lj-920-fix-dangling-reference-to-ctype'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +  -- luacheck: no global
> +  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
> +})
> +
> +test:plan(1)
> +
> +-- This test demonstrates LuaJIT's incorrect behaviour when the
> +-- reallocation of `cts->tab` strikes during the recording of the
> +-- cdata metamethod arithmetic.
> +-- The test fails under ASAN.
> +
> +-- XXX: Just some C functions to be casted. There is no need to
> +-- declare their prototypes correctly.
> +ffi.cdef[[
> +  int malloc(void);
> +  int fprintf(void);
> +  int printf(void);
> +  int memset(void);
> +  int memcpy(void);
> +  int memmove(void);
> +  int getppid(void);
> +]]
> +
> +local cfunc_type = ffi.metatype(ffi.typeof('struct {int a;}'), {
> +  -- Just some metatable with reloaded arithmetic operator.
> +  __add = function(o1, _) return o1 end
> +})
> +-- Just some cdata with metamethod.
> +local test_value = cfunc_type(1)
> +
> +-- XXX: structure to set `cts->top` to 112.
> +local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
> +
> +-- Anchor table to prevent cdata objects from being collected.
> +local anchor = {}
> +-- Each call to this function grows `cts->top` by 3.
> +local function save_new_func(func)
> +  anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
> +end
> +
> +save_new_func(ffi.C.fprintf) -- `cts->top` = 112
> +save_new_func(ffi.C.printf)  -- `cts->top` = 115
> +save_new_func(ffi.C.memset)  -- `cts->top` = 118
> +save_new_func(ffi.C.memcpy)  -- `cts->top` = 121
> +save_new_func(ffi.C.malloc)  -- `cts->top` = 124
> +
> +-- Assertions to check the `cts->top` value and step between
> +-- calls.
> +assert(ffi.typeinfo(124), 'cts->top >= 124')
> +assert(not ffi.typeinfo(125), 'cts->top < 125')
> +
> +save_new_func(ffi.C.memmove) -- `cts->top` = 127
> +
> +jit.opt.start('hotloop=1')
> +
> +-- Just some function to record trace and cause reallocation.
> +local function recorded_func(func_arg)
> +  local res = test_value + func_arg
> +  return res
> +end
> +recorded_func(ffi.C.malloc)
> +
> +assert(ffi.typeinfo(127), 'cts->top >= 127')
> +assert(not ffi.typeinfo(128), 'cts->top < 128')
> +
> +-- Last call to grow `cts->top` up to 129, so this causes
> +-- `cts->tab` reallocation. Need to use different functions as
> +-- an argument.
> +recorded_func(ffi.C.getppid)
> +
> +test:ok(true, 'no heap-use-after-free in recff_cdata_arith')
> +
> +test:done(true)
> -- 
> 2.42.0
> 

Re: [Tarantool-patches] [PATCH luajit 4/6] FFI: Fix dangling reference to CType.

[Maxim Kokryashkin via Tarantool-patches]

Hi again!
I've understood why the assertion after reallocation is not needed,
so you can omit this commit.

Best regards,
Maxim Kokryashkin

Re: [Tarantool-patches] [PATCH luajit 6/6] FFI: Fix dangling reference to CType in carith_checkarg().

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
LGTM, after fixing the same comments, as for the previous patches.
On Mon, Oct 23, 2023 at 12:22:06PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Reported by Sergey Kaplun.
> 
> (cherry-picked from commit db944b2b56c86fcf133745976763604d96110285)
> 
> During of an arithmetic operation with a cdata function object and some
> cdata value in `carith_checkarg()`, reallocation of `cts->tab` in
> `lj_ctype_intern()` may occur. In that case, the reference to the first
> `CType` object (`ca->ct[0]`) becomes invalid. This patch saves the
> `CTypeID` of this object and gets its `CType` again after possible
> reallocation.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#9145
> ---
>  src/lj_carith.c                               |  4 ++
>  ...8-fix-dangling-reference-to-ctype.test.lua | 67 +++++++++++++++++++
>  2 files changed, 71 insertions(+)
>  create mode 100644 test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
> 
> diff --git a/src/lj_carith.c b/src/lj_carith.c
> index 4ae1e9ee..4e1d450a 100644
> --- a/src/lj_carith.c
> +++ b/src/lj_carith.c
> @@ -44,9 +44,13 @@ static int carith_checkarg(lua_State *L, CTState *cts, CDArith *ca)
>  	p = (uint8_t *)cdata_getptr(p, ct->size);
>  	if (ctype_isref(ct->info)) ct = ctype_rawchild(cts, ct);
>        } else if (ctype_isfunc(ct->info)) {
> +	CTypeID id0 = i ? ctype_typeid(cts, ca->ct[0]) : 0;
>  	p = (uint8_t *)*(void **)p;
>  	ct = ctype_get(cts,
>  	  lj_ctype_intern(cts, CTINFO(CT_PTR, CTALIGN_PTR|id), CTSIZE_PTR));
> +	if (i) {  /* cts->tab may have been reallocated. */
> +	  ca->ct[0] = ctype_get(cts, id0);
> +	}
>        }
>        if (ctype_isenum(ct->info)) ct = ctype_child(cts, ct);
>        ca->ct[i] = ct;
> diff --git a/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
> new file mode 100644
> index 00000000..43a39886
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
> @@ -0,0 +1,67 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local test = tap.test('lj-1108-fix-dangling-reference-to-ctype'):skipcond({
> +  -- luacheck: no global
> +  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
> +})
> +
> +test:plan(1)
> +
> +-- This test demonstrates LuaJIT's incorrect behaviour when the
> +-- reallocation of `cts->tab` strikes during the arithmetic cdata
> +-- metamethod.
> +-- The test fails under ASAN.
> +
> +-- XXX: Just some C functions to be casted. There is no need to
> +-- declare their prototypes correctly.
> +ffi.cdef[[
> +  int malloc(void);
> +  int fprintf(void);
> +  int printf(void);
> +  int memset(void);
> +  int memcpy(void);
> +  int memmove(void);
> +  int getppid(void);
> +]]
> +
> +local cfunc_type = ffi.metatype(ffi.typeof('struct {int a;}'), {
> +  -- Just some metatable with reloaded arithmetic operator.
> +  __add = function(o1, _) return o1 end
> +})
> +-- Just some cdata with metamethod.
> +local test_value = cfunc_type(1)
> +
> +-- XXX: structure to set `cts->top` to 112.
> +local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
> +
> +-- Anchor table to prevent cdata objects from being collected.
> +local anchor = {}
> +-- Each call to this function grows `cts->top` by 3.
> +local function save_new_func(func)
> +  anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
> +end
> +
> +save_new_func(ffi.C.fprintf) -- `cts->top` = 112
> +save_new_func(ffi.C.printf)  -- `cts->top` = 115
> +save_new_func(ffi.C.memset)  -- `cts->top` = 118
> +save_new_func(ffi.C.memcpy)  -- `cts->top` = 121
> +save_new_func(ffi.C.malloc)  -- `cts->top` = 124
> +
> +-- Assertions to check the `cts->top` value and step between
> +-- calls.
> +assert(ffi.typeinfo(124), 'cts->top >= 124')
> +assert(not ffi.typeinfo(125), 'cts->top < 125')
> +
> +save_new_func(ffi.C.memmove) -- `cts->top` = 127
> +
> +assert(ffi.typeinfo(127), 'cts->top >= 127')
> +assert(not ffi.typeinfo(128), 'cts->top < 128')
> +
> +-- Just check cdata arith metamethod. The function argument should
> +-- be the second because dangling reference is the CType of the
> +-- first argumnent.
> +_ = test_value + ffi.C.getppid
> +
> +test:ok(true, 'no heap-use-after-free in carith_checkarg')
> +
> +test:done(true)
> -- 
> 2.42.0
> 

Re: [Tarantool-patches] [PATCH luajit 2/6] FFI: Fix missing cts->L initialization in argv2ctype().

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Please, consider my answer below.

On 24.10.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits regarding the commit message.
> On Mon, Oct 23, 2023 at 12:22:02PM +0300, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> > 
> > (cherry-picked from commit 50d6883e6027c4c2f9a5e495fee6b7fff1bd73c9)
> Incorrect hash, should be: a622e2eb559c823d90c7af85935ca63706e4593d
> > 
> > When start trace recording without an initialized `L` in CType State
> > (possible if the recording is started before any `ffi` library usage),
> > the corresponding assertion fails in the `lj_ctype_intern()`. This patch
> > adds missing initialization during recording.
> 
> Please explicitly mention that the test case fails only when the LuaJIT
> is built with -DLUA_USE_ASSERT=ON.

> > the corresponding assertion fails in the `lj_ctype_intern()`. This patch
It's obvious since there are no assertions if this option is set to OFF.

So, ignoring.

> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#9145
> > ---
> <snipped>
> Best regards,
> Maxim Kokryashkin 
>  

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 4/6] FFI: Fix dangling reference to CType.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
See my answers below.

On 25.10.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> On Mon, Oct 23, 2023 at 12:22:04PM +0300, Sergey Kaplun wrote:

<snipped>

> > diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
> > new file mode 100644
> > index 00000000..c0e2c07b
> > --- /dev/null
> > +++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
> > @@ -0,0 +1,59 @@
> > +local tap = require('tap')
> > +local ffi = require('ffi')
> > +local test = tap.test('fix-dangling-reference-to-ctype'):skipcond({
> > +  -- luacheck: no global
> > +  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
> > +})
> > +
> > +test:plan(1)
> > +
> > +-- This test demonstrates LuaJIT's incorrect behaviour when the
> > +-- reallocation of `cts->tab` strikes during the conversion of a
> > +-- TValue (cdata function pointer) to a C type.
> > +-- The test fails under ASAN.
> Let's change the last sentence to 'Before the patch the test fails only
> under ASAN' because now it is a bit misleading. 

Reworded, thanks!

> > +
> > +-- XXX: Just some C functions to be casted. There is no need to
> > +-- declare their prototypes correctly.
> > +ffi.cdef[[
> > +  int malloc(void);
> > +  int fprintf(void);
> > +  int printf(void);
> > +  int memset(void);
> > +  int memcpy(void);
> > +  int memmove(void);
> > +  int getppid(void);
> > +]]
> > +
> > +-- XXX: structure to set `cts->top` to 110.
> > +local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
> > +
> > +-- Anchor table to prevent cdata objects from being collected.
> > +local anchor = {}
> > +-- Each call to this function grows `cts->top` by 3.
> Please drop a comment, referring to a point in sources, so the size
> of the growth becomes obvious.

Added. See the iterative patch below.

> 
> > +local function save_new_func(func)
> > +  anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
> > +end
> > +
> > +save_new_func(ffi.C.malloc)  -- `cts->top` = 110
> > +save_new_func(ffi.C.fprintf) -- `cts->top` = 113
> > +save_new_func(ffi.C.printf)  -- `cts->top` = 116
> > +save_new_func(ffi.C.memset)  -- `cts->top` = 119
> > +save_new_func(ffi.C.memcpy)  -- `cts->top` = 122
> 
> Is it possible to bring us to this value of `cts->top`
> with a structure?

I haven't tried it, but this structure will be too big and hardly
maintained, so I prefer the following way. I suppose that there is no
need to comment this part, so the only comment I left is about the first
alignment.

> > +
> > +-- Assertions to check the `cts->top` value and step between
> > +-- calls.
> > +assert(ffi.typeinfo(122), 'cts->top >= 122')
> > +assert(not ffi.typeinfo(123), 'cts->top < 123')
> > +
> > +save_new_func(ffi.C.memmove) -- `cts->top` = 125
> > +
> > +assert(ffi.typeinfo(125), 'cts->top >= 125')
> > +assert(not ffi.typeinfo(126), 'cts->top < 126')
> > +
> > +-- Last call to grow `cts->top` up to 128, so this causes
> > +-- `cts->tab` reallocation.
> > +save_new_func(ffi.C.getppid) -- `cts->top` = 128
> 
> Should we add an extra assertion after reallocation?

Ignored, as you mentioned in the second letter.

> > +
> > +test:ok(true, 'no heap-use-after-free in lj_cconv_ct_tv')
> > +
> > +test:done(true)
> > -- 
> > 2.42.0
> > 

Branch is force pushed:
===================================================================
diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
index c0e2c07b..2ced5779 100644
--- a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
+++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
@@ -10,7 +10,7 @@ test:plan(1)
 -- This test demonstrates LuaJIT's incorrect behaviour when the
 -- reallocation of `cts->tab` strikes during the conversion of a
 -- TValue (cdata function pointer) to a C type.
--- The test fails under ASAN.
+-- Before the patch, the test failed only under ASAN.
 
 -- XXX: Just some C functions to be casted. There is no need to
 -- declare their prototypes correctly.
@@ -30,6 +30,9 @@ local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
 -- Anchor table to prevent cdata objects from being collected.
 local anchor = {}
 -- Each call to this function grows `cts->top` by 3.
+-- `lj_ctype_new()` and `lj_ctype_intern()` during the parsing of
+-- the `CType` declaration in the `ffi.cast()` plus
+-- `lj_ctype_intern()` during the conversion to another `CType`.
 local function save_new_func(func)
   anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
 end
===================================================================

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 5/6] FFI: Fix dangling reference to CType. Improve checks.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!

On 25.10.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, after answering the same questions as for the previous patch.
> On Mon, Oct 23, 2023 at 12:22:05PM +0300, Sergey Kaplun wrote:

See the iterative patch below, branch is force pushed:
===================================================================
diff --git a/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
index a7e35888..0b8be04f 100644
--- a/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
+++ b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
@@ -11,7 +11,7 @@ test:plan(1)
 -- This test demonstrates LuaJIT's incorrect behaviour when the
 -- reallocation of `cts->tab` strikes during the recording of the
 -- cdata metamethod arithmetic.
--- The test fails under ASAN.
+-- Before the patch, the test failed only under ASAN.
 
 -- XXX: Just some C functions to be casted. There is no need to
 -- declare their prototypes correctly.
@@ -38,6 +38,9 @@ local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
 -- Anchor table to prevent cdata objects from being collected.
 local anchor = {}
 -- Each call to this function grows `cts->top` by 3.
+-- `lj_ctype_new()` and `lj_ctype_intern()` during the parsing of
+-- the `CType` declaration in the `ffi.cast()` plus
+-- `lj_ctype_intern()` during the conversion to another `CType`.
 local function save_new_func(func)
   anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
 end
===================================================================

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 6/6] FFI: Fix dangling reference to CType in carith_checkarg().

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!

On 25.10.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> LGTM, after fixing the same comments, as for the previous patches.

See the iterative patch below, branch is force-pushed.
===================================================================
diff --git a/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
index 43a39886..a077344e 100644
--- a/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
+++ b/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
@@ -10,7 +10,7 @@ test:plan(1)
 -- This test demonstrates LuaJIT's incorrect behaviour when the
 -- reallocation of `cts->tab` strikes during the arithmetic cdata
 -- metamethod.
--- The test fails under ASAN.
+-- Before the patch, the test failed only under ASAN.
 
 -- XXX: Just some C functions to be casted. There is no need to
 -- declare their prototypes correctly.
@@ -37,6 +37,9 @@ local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
 -- Anchor table to prevent cdata objects from being collected.
 local anchor = {}
 -- Each call to this function grows `cts->top` by 3.
+-- `lj_ctype_new()` and `lj_ctype_intern()` during the parsing of
+-- the `CType` declaration in the `ffi.cast()` plus
+-- `lj_ctype_intern()` during the conversion to another `CType`.
 local function save_new_func(func)
   anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
 end
===================================================================

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 4/6] FFI: Fix dangling reference to CType.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi!
Thanks for the fixes!
LGTM
 
--
Best regards,
Maxim Kokryashkin
 
  
>Среда, 25 октября 2023, 13:47 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Maxim!
>Thanks for the review!
>See my answers below.
>
>On 25.10.23, Maxim Kokryashkin wrote:
>> Hi, Sergey!
>> Thanks for the patch!
>> Please consider my comments below.
>> On Mon, Oct 23, 2023 at 12:22:04PM +0300, Sergey Kaplun wrote:
>
><snipped>
>
>> > diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
>> > new file mode 100644
>> > index 00000000..c0e2c07b
>> > --- /dev/null
>> > +++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
>> > @@ -0,0 +1,59 @@
>> > +local tap = require('tap')
>> > +local ffi = require('ffi')
>> > +local test = tap.test('fix-dangling-reference-to-ctype'):skipcond({
>> > + -- luacheck: no global
>> > + ['Impossible to predict the value of cts->top'] = _TARANTOOL,
>> > +})
>> > +
>> > +test:plan(1)
>> > +
>> > +-- This test demonstrates LuaJIT's incorrect behaviour when the
>> > +-- reallocation of `cts->tab` strikes during the conversion of a
>> > +-- TValue (cdata function pointer) to a C type.
>> > +-- The test fails under ASAN.
>> Let's change the last sentence to 'Before the patch the test fails only
>> under ASAN' because now it is a bit misleading.
>
>Reworded, thanks!
>
>> > +
>> > +-- XXX: Just some C functions to be casted. There is no need to
>> > +-- declare their prototypes correctly.
>> > +ffi.cdef[[
>> > + int malloc(void);
>> > + int fprintf(void);
>> > + int printf(void);
>> > + int memset(void);
>> > + int memcpy(void);
>> > + int memmove(void);
>> > + int getppid(void);
>> > +]]
>> > +
>> > +-- XXX: structure to set `cts->top` to 110.
>> > +local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
>> > +
>> > +-- Anchor table to prevent cdata objects from being collected.
>> > +local anchor = {}
>> > +-- Each call to this function grows `cts->top` by 3.
>> Please drop a comment, referring to a point in sources, so the size
>> of the growth becomes obvious.
>
>Added. See the iterative patch below.
>
>>
>> > +local function save_new_func(func)
>> > + anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
>> > +end
>> > +
>> > +save_new_func(ffi.C.malloc) -- `cts->top` = 110
>> > +save_new_func(ffi.C.fprintf) -- `cts->top` = 113
>> > +save_new_func(ffi.C.printf) -- `cts->top` = 116
>> > +save_new_func(ffi.C.memset) -- `cts->top` = 119
>> > +save_new_func(ffi.C.memcpy) -- `cts->top` = 122
>>
>> Is it possible to bring us to this value of `cts->top`
>> with a structure?
>
>I haven't tried it, but this structure will be too big and hardly
>maintained, so I prefer the following way. I suppose that there is no
>need to comment this part, so the only comment I left is about the first
>alignment.
>
>> > +
>> > +-- Assertions to check the `cts->top` value and step between
>> > +-- calls.
>> > +assert(ffi.typeinfo(122), 'cts->top >= 122')
>> > +assert(not ffi.typeinfo(123), 'cts->top < 123')
>> > +
>> > +save_new_func(ffi.C.memmove) -- `cts->top` = 125
>> > +
>> > +assert(ffi.typeinfo(125), 'cts->top >= 125')
>> > +assert(not ffi.typeinfo(126), 'cts->top < 126')
>> > +
>> > +-- Last call to grow `cts->top` up to 128, so this causes
>> > +-- `cts->tab` reallocation.
>> > +save_new_func(ffi.C.getppid) -- `cts->top` = 128
>>
>> Should we add an extra assertion after reallocation?
>
>Ignored, as you mentioned in the second letter.
>
>> > +
>> > +test:ok(true, 'no heap-use-after-free in lj_cconv_ct_tv')
>> > +
>> > +test:done(true)
>> > --
>> > 2.42.0
>> >
>
>Branch is force pushed:
>===================================================================
>diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
>index c0e2c07b..2ced5779 100644
>--- a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
>+++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
>@@ -10,7 +10,7 @@ test:plan(1)
> -- This test demonstrates LuaJIT's incorrect behaviour when the
> -- reallocation of `cts->tab` strikes during the conversion of a
> -- TValue (cdata function pointer) to a C type.
>--- The test fails under ASAN.
>+-- Before the patch, the test failed only under ASAN.
> 
> -- XXX: Just some C functions to be casted. There is no need to
> -- declare their prototypes correctly.
>@@ -30,6 +30,9 @@ local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
> -- Anchor table to prevent cdata objects from being collected.
> local anchor = {}
> -- Each call to this function grows `cts->top` by 3.
>+-- `lj_ctype_new()` and `lj_ctype_intern()` during the parsing of
>+-- the `CType` declaration in the `ffi.cast()` plus
>+-- `lj_ctype_intern()` during the conversion to another `CType`.
> local function save_new_func(func)
>   anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
> end
>===================================================================
>
>--
>Best regards,
>Sergey Kaplun
 

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

Re: [Tarantool-patches] [PATCH luajit 5/6] FFI: Fix dangling reference to CType. Improve checks.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
  
>Среда, 25 октября 2023, 13:51 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Maxim!
>Thanks for the review!
>
>On 25.10.23, Maxim Kokryashkin wrote:
>> Hi, Sergey!
>> Thanks for the patch!
>> LGTM, after answering the same questions as for the previous patch.
>> On Mon, Oct 23, 2023 at 12:22:05PM +0300, Sergey Kaplun wrote:
>See the iterative patch below, branch is force pushed:
>===================================================================
>diff --git a/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
>index a7e35888..0b8be04f 100644
>--- a/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
>+++ b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
>@@ -11,7 +11,7 @@ test:plan(1)
> -- This test demonstrates LuaJIT's incorrect behaviour when the
> -- reallocation of `cts->tab` strikes during the recording of the
> -- cdata metamethod arithmetic.
>--- The test fails under ASAN.
>+-- Before the patch, the test failed only under ASAN.
> 
> -- XXX: Just some C functions to be casted. There is no need to
> -- declare their prototypes correctly.
>@@ -38,6 +38,9 @@ local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
> -- Anchor table to prevent cdata objects from being collected.
> local anchor = {}
> -- Each call to this function grows `cts->top` by 3.
>+-- `lj_ctype_new()` and `lj_ctype_intern()` during the parsing of
>+-- the `CType` declaration in the `ffi.cast()` plus
>+-- `lj_ctype_intern()` during the conversion to another `CType`.
> local function save_new_func(func)
>   anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
> end
>===================================================================
>
>--
>Best regards,
>Sergey Kaplun
 

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

Re: [Tarantool-patches] [PATCH luajit 6/6] FFI: Fix dangling reference to CType in carith_checkarg().

[Maxim Kokryashkin via Tarantool-patches]

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


Hi!
Thanks for the fixes!
LGTM
 
--
Best regards,
Maxim Kokryashkin
 
  
>Среда, 25 октября 2023, 13:52 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Maxim!
>Thanks for the review!
>
>On 25.10.23, Maxim Kokryashkin wrote:
>> Hi, Sergey!
>> LGTM, after fixing the same comments, as for the previous patches.
>See the iterative patch below, branch is force-pushed.
>===================================================================
>diff --git a/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
>index 43a39886..a077344e 100644
>--- a/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
>+++ b/test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
>@@ -10,7 +10,7 @@ test:plan(1)
> -- This test demonstrates LuaJIT's incorrect behaviour when the
> -- reallocation of `cts->tab` strikes during the arithmetic cdata
> -- metamethod.
>--- The test fails under ASAN.
>+-- Before the patch, the test failed only under ASAN.
> 
> -- XXX: Just some C functions to be casted. There is no need to
> -- declare their prototypes correctly.
>@@ -37,6 +37,9 @@ local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
> -- Anchor table to prevent cdata objects from being collected.
> local anchor = {}
> -- Each call to this function grows `cts->top` by 3.
>+-- `lj_ctype_new()` and `lj_ctype_intern()` during the parsing of
>+-- the `CType` declaration in the `ffi.cast()` plus
>+-- `lj_ctype_intern()` during the conversion to another `CType`.
> local function save_new_func(func)
>   anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
> end
>===================================================================
>
>--
>Best regards,
>Sergey Kaplun
 

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

Re: [Tarantool-patches] [PATCH luajit 1/6] Abstract out on-demand loading of FFI library.

[Sergey Bronnikov via Tarantool-patches]

Hello, Sergey


thanks for the patch! LGTM


On 10/23/23 12:22, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> (cherry-picked from commit 50d6883e6027c4c2f9a5e495fee6b7fff1bd73c9)
>
> This patch introduces the macro `ctype_loadffi(L)` to avoid code
> duplication while loading the FFI library.
>
> Sergey Kaplun:
> * added the description
>
> Part of tarantool/tarantool#9145
> ---
>   src/lib_jit.c   |  6 +-----
>   src/lj_bcread.c |  6 +-----
>   src/lj_ctype.h  | 10 ++++++++++
>   src/lj_lex.c    |  6 +-----
>   4 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/src/lib_jit.c b/src/lib_jit.c
> index b3c1c93c..f705f334 100644
> --- a/src/lib_jit.c
> +++ b/src/lib_jit.c
> @@ -339,11 +339,7 @@ LJLIB_CF(jit_util_tracek)
>         ir = &T->ir[ir->op1];
>       }
>   #if LJ_HASFFI
> -    if (ir->o == IR_KINT64 && !ctype_ctsG(G(L))) {
> -      ptrdiff_t oldtop = savestack(L, L->top);
> -      luaopen_ffi(L);  /* Load FFI library on-demand. */
> -      L->top = restorestack(L, oldtop);
> -    }
> +    if (ir->o == IR_KINT64) ctype_loadffi(L);
>   #endif
>       lj_ir_kvalue(L, L->top-2, ir);
>       setintV(L->top-1, (int32_t)irt_type(ir->t));
> diff --git a/src/lj_bcread.c b/src/lj_bcread.c
> index cddf6ff1..4915240f 100644
> --- a/src/lj_bcread.c
> +++ b/src/lj_bcread.c
> @@ -414,11 +414,7 @@ static int bcread_header(LexState *ls)
>     if ((flags & BCDUMP_F_FFI)) {
>   #if LJ_HASFFI
>       lua_State *L = ls->L;
> -    if (!ctype_ctsG(G(L))) {
> -      ptrdiff_t oldtop = savestack(L, L->top);
> -      luaopen_ffi(L);  /* Load FFI library on-demand. */
> -      L->top = restorestack(L, oldtop);
> -    }
> +    ctype_loadffi(L);
>   #else
>       return 0;
>   #endif
> diff --git a/src/lj_ctype.h b/src/lj_ctype.h
> index c4f3bdde..fce29409 100644
> --- a/src/lj_ctype.h
> +++ b/src/lj_ctype.h
> @@ -389,6 +389,16 @@ static LJ_AINLINE CTState *ctype_cts(lua_State *L)
>     return cts;
>   }
>   
> +/* Load FFI library on-demand. */
> +#define ctype_loadffi(L) \
> +  do { \
> +    if (!ctype_ctsG(G(L))) { \
> +      ptrdiff_t oldtop = (char *)L->top - mref(L->stack, char); \
> +      luaopen_ffi(L); \
> +      L->top = (TValue *)(mref(L->stack, char) + oldtop); \
> +    } \
> +  } while (0)
> +
>   /* Save and restore state of C type table. */
>   #define LJ_CTYPE_SAVE(cts)	CTState savects_ = *(cts)
>   #define LJ_CTYPE_RESTORE(cts) \
> diff --git a/src/lj_lex.c b/src/lj_lex.c
> index cef3c683..7c6319cc 100644
> --- a/src/lj_lex.c
> +++ b/src/lj_lex.c
> @@ -112,11 +112,7 @@ static void lex_number(LexState *ls, TValue *tv)
>       GCcdata *cd;
>       lj_assertLS(fmt == STRSCAN_I64 || fmt == STRSCAN_U64 || fmt == STRSCAN_IMAG,
>   		"unexpected number format %d", fmt);
> -    if (!ctype_ctsG(G(L))) {
> -      ptrdiff_t oldtop = savestack(L, L->top);
> -      luaopen_ffi(L);  /* Load FFI library on-demand. */
> -      L->top = restorestack(L, oldtop);
> -    }
> +    ctype_loadffi(L);
>       if (fmt == STRSCAN_IMAG) {
>         cd = lj_cdata_new_(L, CTID_COMPLEX_DOUBLE, 2*sizeof(double));
>         ((double *)cdataptr(cd))[0] = 0;

Re: [Tarantool-patches] [PATCH luajit 2/6] FFI: Fix missing cts->L initialization in argv2ctype().

[Sergey Bronnikov via Tarantool-patches]

Hello, Sergey

thanks for the patch! LGTM

On 10/23/23 12:22, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> (cherry-picked from commit 50d6883e6027c4c2f9a5e495fee6b7fff1bd73c9)
>
> When start trace recording without an initialized `L` in CType State
> (possible if the recording is started before any `ffi` library usage),
> the corresponding assertion fails in the `lj_ctype_intern()`. This patch
> adds missing initialization during recording.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9145
> ---
>   src/lj_crecord.c                                  |  2 +-
>   .../fix-argv2ctype-cts-L-init.test.lua            | 15 +++++++++++++++
>   .../fix-argv2ctype-cts-L-init/script.lua          | 14 ++++++++++++++
>   3 files changed, 30 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua
>   create mode 100644 test/tarantool-tests/fix-argv2ctype-cts-L-init/script.lua
>
> diff --git a/src/lj_crecord.c b/src/lj_crecord.c
> index e1d1110f..10d1dc70 100644
> --- a/src/lj_crecord.c
> +++ b/src/lj_crecord.c
> @@ -78,7 +78,7 @@ static CTypeID argv2ctype(jit_State *J, TRef tr, cTValue *o)
>       /* Specialize to the string containing the C type declaration. */
>       emitir(IRTG(IR_EQ, IRT_STR), tr, lj_ir_kstr(J, s));
>       cp.L = J->L;
> -    cp.cts = ctype_ctsG(J2G(J));
> +    cp.cts = ctype_cts(J->L);
>       oldtop = cp.cts->top;
>       cp.srcname = strdata(s);
>       cp.p = strdata(s);
> diff --git a/test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua b/test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua
> new file mode 100644
> index 00000000..ee45e424
> --- /dev/null
> +++ b/test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua
> @@ -0,0 +1,15 @@
> +local tap = require('tap')
> +local test = tap.test('fix-argv2ctype-cts-L-init'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +-- Loading of 'tap' module initialize `cts->L` during parsing.
> +-- Run standalone script for testing.
> +local script = require('utils').exec.makecmd(arg)
> +
> +test:plan(1)
> +
> +local output = script()
> +test:is(output, 'OK', 'correct recording with uninitialized cts->L')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/fix-argv2ctype-cts-L-init/script.lua b/test/tarantool-tests/fix-argv2ctype-cts-L-init/script.lua
> new file mode 100644
> index 00000000..2131385a
> --- /dev/null
> +++ b/test/tarantool-tests/fix-argv2ctype-cts-L-init/script.lua
> @@ -0,0 +1,14 @@
> +local ffi = require('ffi')
> +
> +jit.opt.start('hotloop=1')
> +
> +local i = 1
> +-- Use `while` to start compilation of the trace at the first
> +-- iteration, before `ffi.new()` is called, so `cts->L` is
> +-- uninitialized.
> +while i < 3 do
> +  ffi.new('uint64_t', i)
> +  i = i + 1
> +end
> +
> +print('OK')

Re: [Tarantool-patches] [PATCH luajit 3/6] FFI: Ensure returned string is alive in ffi.typeinfo().

[Sergey Bronnikov via Tarantool-patches]

Hello, Sergey

thanks for the patch! LGTM

On 10/23/23 12:22, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by zhuizhuhaomeng.
>
> (cherry-picked from commit 94a40bb238092e73f3dc0c3626911a7efa997c22)
>
> `ffi.typeinfo()` doesn't check that the string containing the name type
> is alive when it sets this string to the returned table. This leads to
> the corresponding assertion failure in `checklivetv()`. This patch flips
> the white colour of the stored string to resurrect it.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9145


<snipped>

Re: [Tarantool-patches] [PATCH luajit 4/6] FFI: Fix dangling reference to CType.

[Sergey Bronnikov via Tarantool-patches]

Hello, Sergey


thanks for the patch!

the test is passed with reverted fix and LuaJIT built under ASAN:


$ nm ./build/src/luajit | grep __asan_init
                  U __asan_init
$ ./build/src/luajit 
test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
TAP version 13
1..1
ok - no heap-use-after-free in lj_cconv_ct_tv
$

On 10/23/23 12:22, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> (cherry-picked from commit ae533e3a6c009b5df79b11cd5787d249202fa69c)
>
> During the conversion of a cdata function object to some cdata value in
> `lj_cconv_ct_tv()`, reallocation of `cts->tab` in `lj_ctype_intern()`
> may occur. In that case, the reference to the `CType` object becomes
> invalid. This patch saves the `CTypeID` of the given function and gets
> its `CType` again after possible reallocation.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9145
> ---
>   src/lj_cconv.c                                |  2 +
>   .../fix-dangling-reference-to-ctype.test.lua  | 59 +++++++++++++++++++
>   2 files changed, 61 insertions(+)
>   create mode 100644 test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
>
> diff --git a/src/lj_cconv.c b/src/lj_cconv.c
> index 37c88852..94ca93bb 100644
> --- a/src/lj_cconv.c
> +++ b/src/lj_cconv.c
> @@ -568,7 +568,9 @@ void lj_cconv_ct_tv(CTState *cts, CType *d,
>       }
>       s = ctype_raw(cts, sid);
>       if (ctype_isfunc(s->info)) {
> +      CTypeID did = ctype_typeid(cts, d);
>         sid = lj_ctype_intern(cts, CTINFO(CT_PTR, CTALIGN_PTR|sid), CTSIZE_PTR);
> +      d = ctype_get(cts, did);  /* cts->tab may have been reallocated. */
>       } else {
>         if (ctype_isenum(s->info)) s = ctype_child(cts, s);
>         goto doconv;
> diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
> new file mode 100644
> index 00000000..c0e2c07b
> --- /dev/null
> +++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
> @@ -0,0 +1,59 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local test = tap.test('fix-dangling-reference-to-ctype'):skipcond({
> +  -- luacheck: no global
> +  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
> +})
> +
> +test:plan(1)
> +
> +-- This test demonstrates LuaJIT's incorrect behaviour when the
> +-- reallocation of `cts->tab` strikes during the conversion of a
> +-- TValue (cdata function pointer) to a C type.
> +-- The test fails under ASAN.
> +
> +-- XXX: Just some C functions to be casted. There is no need to
> +-- declare their prototypes correctly.
> +ffi.cdef[[
> +  int malloc(void);
> +  int fprintf(void);
> +  int printf(void);
> +  int memset(void);
> +  int memcpy(void);
> +  int memmove(void);
> +  int getppid(void);
> +]]
> +
> +-- XXX: structure to set `cts->top` to 110.
> +local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
> +
> +-- Anchor table to prevent cdata objects from being collected.
> +local anchor = {}
> +-- Each call to this function grows `cts->top` by 3.
> +local function save_new_func(func)
> +  anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
> +end
> +
> +save_new_func(ffi.C.malloc)  -- `cts->top` = 110
> +save_new_func(ffi.C.fprintf) -- `cts->top` = 113
> +save_new_func(ffi.C.printf)  -- `cts->top` = 116
> +save_new_func(ffi.C.memset)  -- `cts->top` = 119
> +save_new_func(ffi.C.memcpy)  -- `cts->top` = 122
> +
> +-- Assertions to check the `cts->top` value and step between
> +-- calls.
> +assert(ffi.typeinfo(122), 'cts->top >= 122')
> +assert(not ffi.typeinfo(123), 'cts->top < 123')
> +
> +save_new_func(ffi.C.memmove) -- `cts->top` = 125
> +
> +assert(ffi.typeinfo(125), 'cts->top >= 125')
> +assert(not ffi.typeinfo(126), 'cts->top < 126')
> +
> +-- Last call to grow `cts->top` up to 128, so this causes
> +-- `cts->tab` reallocation.
> +save_new_func(ffi.C.getppid) -- `cts->top` = 128
> +
> +test:ok(true, 'no heap-use-after-free in lj_cconv_ct_tv')
> +
> +test:done(true)

Re: [Tarantool-patches] [PATCH luajit 5/6] FFI: Fix dangling reference to CType. Improve checks.

[Sergey Bronnikov via Tarantool-patches]

Hello, Sergey

thanks for the patch! See comments below

On 10/23/23 12:22, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by elmknot.
>
> (cherry-picked from commit cc96ab9d513582703f8663a8775a935b56db32b7)
>
> During the recording of an arithmetic operation with a cdata function
> object and some cdata value in `recff_cdata_arith()`, reallocation of
> `cts->tab` in `lj_ctype_intern()` may occur. In that case, the reference
> to the first `CType` object (`s[0]`) becomes invalid. This patch saves
> the `CTypeID` of this object and gets its `CType` again after possible
> reallocation.
>
> Also, this commit fills `cts->tab` memory with zeros before being freed
> when `-DLUAJIT_CTYPE_CHECK_ANCHOR` is defined. It helps to observe
> assertion failures in case this memory is used after free.

failed to reproduce a bug with reverted fix and LuaJIT built with a flag:

$ CMAKE_C_FLAGS=-DLUAJIT_CTYPE_CHECK_ANCHOR 
CFLAGS=-DLUAJIT_CTYPE_CHECK_ANCHOR cmake -S . -B build 
-DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUAJIT_USE_ASAN=ON
$ ./build/src/luajit 
test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
TAP version 13
1..1
ok - no heap-use-after-free in recff_cdata_arith

>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9145
> ---
>   src/lj_crecord.c                              |  4 +
>   src/lj_ctype.c                                | 12 +++
>   ...0-fix-dangling-reference-to-ctype.test.lua | 77 +++++++++++++++++++
>   3 files changed, 93 insertions(+)
>   create mode 100644 test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
>
> diff --git a/src/lj_crecord.c b/src/lj_crecord.c
> index 10d1dc70..e17e512f 100644
> --- a/src/lj_crecord.c
> +++ b/src/lj_crecord.c
> @@ -1502,9 +1502,13 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
>   	if (ctype_isenum(ct->info)) ct = ctype_child(cts, ct);
>   	goto ok;
>         } else if (ctype_isfunc(ct->info)) {
> +	CTypeID id0 = i ? ctype_typeid(cts, s[0]) : 0;
>   	tr = emitir(IRT(IR_FLOAD, IRT_PTR), tr, IRFL_CDATA_PTR);
>   	ct = ctype_get(cts,
>   	  lj_ctype_intern(cts, CTINFO(CT_PTR, CTALIGN_PTR|id), CTSIZE_PTR));
> +	if (i) {
> +	  s[0] = ctype_get(cts, id0);  /* cts->tab may have been reallocated. */
> +	}
>   	goto ok;
>         } else {
>   	tr = emitir(IRT(IR_ADD, IRT_PTR), tr, lj_ir_kintp(J, sizeof(GCcdata)));
> diff --git a/src/lj_ctype.c b/src/lj_ctype.c
> index a42e3d60..0874fa61 100644
> --- a/src/lj_ctype.c
> +++ b/src/lj_ctype.c
> @@ -191,8 +191,20 @@ CTypeID lj_ctype_intern(CTState *cts, CTInfo info, CTSize size)
>     }
>     id = cts->top;
>     if (LJ_UNLIKELY(id >= cts->sizetab)) {
> +#ifdef LUAJIT_CTYPE_CHECK_ANCHOR
> +    CType *ct;
> +#endif
>       if (id >= CTID_MAX) lj_err_msg(cts->L, LJ_ERR_TABOV);
> +#ifdef LUAJIT_CTYPE_CHECK_ANCHOR
> +    ct = lj_mem_newvec(cts->L, id+1, CType);
> +    memcpy(ct, cts->tab, id*sizeof(CType));
> +    memset(cts->tab, 0, id*sizeof(CType));
> +    lj_mem_freevec(cts->g, cts->tab, cts->sizetab, CType);
> +    cts->tab = ct;
> +    cts->sizetab = id+1;
> +#else
>       lj_mem_growvec(cts->L, cts->tab, cts->sizetab, CTID_MAX, CType);
> +#endif
>     }
>     cts->top = id+1;
>     cts->tab[id].info = info;
> diff --git a/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> new file mode 100644
> index 00000000..a7e35888
> --- /dev/null
> +++ b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> @@ -0,0 +1,77 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local test = tap.test('lj-920-fix-dangling-reference-to-ctype'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +  -- luacheck: no global
> +  ['Impossible to predict the value of cts->top'] = _TARANTOOL,

Let's declare _TARANTOOL as global in .luacheckrc instead of suppressing 
warning inline every time.

About 7 tests suppress it.

> +})
> +
> +test:plan(1)
> +
> +-- This test demonstrates LuaJIT's incorrect behaviour when the
> +-- reallocation of `cts->tab` strikes during the recording of the
> +-- cdata metamethod arithmetic.
> +-- The test fails under ASAN.
> +
> +-- XXX: Just some C functions to be casted. There is no need to
> +-- declare their prototypes correctly.
> +ffi.cdef[[
> +  int malloc(void);
> +  int fprintf(void);
> +  int printf(void);
> +  int memset(void);
> +  int memcpy(void);
> +  int memmove(void);
> +  int getppid(void);
> +]]
> +
> +local cfunc_type = ffi.metatype(ffi.typeof('struct {int a;}'), {
> +  -- Just some metatable with reloaded arithmetic operator.
> +  __add = function(o1, _) return o1 end
> +})
> +-- Just some cdata with metamethod.
> +local test_value = cfunc_type(1)
> +
> +-- XXX: structure to set `cts->top` to 112.
> +local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
> +
> +-- Anchor table to prevent cdata objects from being collected.
> +local anchor = {}
> +-- Each call to this function grows `cts->top` by 3.
> +local function save_new_func(func)
> +  anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
> +end
> +
> +save_new_func(ffi.C.fprintf) -- `cts->top` = 112
> +save_new_func(ffi.C.printf)  -- `cts->top` = 115
> +save_new_func(ffi.C.memset)  -- `cts->top` = 118
> +save_new_func(ffi.C.memcpy)  -- `cts->top` = 121
> +save_new_func(ffi.C.malloc)  -- `cts->top` = 124
> +
> +-- Assertions to check the `cts->top` value and step between
> +-- calls.
> +assert(ffi.typeinfo(124), 'cts->top >= 124')
> +assert(not ffi.typeinfo(125), 'cts->top < 125')
> +
> +save_new_func(ffi.C.memmove) -- `cts->top` = 127
> +
> +jit.opt.start('hotloop=1')
> +
> +-- Just some function to record trace and cause reallocation.
> +local function recorded_func(func_arg)
> +  local res = test_value + func_arg
> +  return res
> +end
> +recorded_func(ffi.C.malloc)
> +
> +assert(ffi.typeinfo(127), 'cts->top >= 127')
> +assert(not ffi.typeinfo(128), 'cts->top < 128')
> +
> +-- Last call to grow `cts->top` up to 129, so this causes
> +-- `cts->tab` reallocation. Need to use different functions as
> +-- an argument.
> +recorded_func(ffi.C.getppid)
> +
> +test:ok(true, 'no heap-use-after-free in recff_cdata_arith')
> +
> +test:done(true)

Re: [Tarantool-patches] [PATCH luajit 5/6] FFI: Fix dangling reference to CType. Improve checks.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Please, consider my answers below.

On 29.11.23, Sergey Bronnikov wrote:
> Hello, Sergey
> 
> thanks for the patch! See comments below
> 
> On 10/23/23 12:22, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Reported by elmknot.
> >
> > (cherry-picked from commit cc96ab9d513582703f8663a8775a935b56db32b7)
> >
> > During the recording of an arithmetic operation with a cdata function
> > object and some cdata value in `recff_cdata_arith()`, reallocation of
> > `cts->tab` in `lj_ctype_intern()` may occur. In that case, the reference
> > to the first `CType` object (`s[0]`) becomes invalid. This patch saves
> > the `CTypeID` of this object and gets its `CType` again after possible
> > reallocation.
> >
> > Also, this commit fills `cts->tab` memory with zeros before being freed
> > when `-DLUAJIT_CTYPE_CHECK_ANCHOR` is defined. It helps to observe
> > assertion failures in case this memory is used after free.
> 
> failed to reproduce a bug with reverted fix and LuaJIT built with a flag:
> 
> $ CMAKE_C_FLAGS=-DLUAJIT_CTYPE_CHECK_ANCHOR 
> CFLAGS=-DLUAJIT_CTYPE_CHECK_ANCHOR cmake -S . -B build 
> -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUAJIT_USE_ASAN=ON

I suppose you forgot the -DLUAJIT_USE_SYSMALLOC=ON
-DLUAJIT_ENABLE_GC64=ON flags. The same is valid for the previous patch.

> $ ./build/src/luajit 
> test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> TAP version 13
> 1..1
> ok - no heap-use-after-free in recff_cdata_arith
> 
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9145
> > ---
> >   src/lj_crecord.c                              |  4 +
> >   src/lj_ctype.c                                | 12 +++
> >   ...0-fix-dangling-reference-to-ctype.test.lua | 77 +++++++++++++++++++
> >   3 files changed, 93 insertions(+)
> >   create mode 100644 test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> >

<snipped>

> > +++ b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> > @@ -0,0 +1,77 @@
> > +local tap = require('tap')
> > +local ffi = require('ffi')
> > +local test = tap.test('lj-920-fix-dangling-reference-to-ctype'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +  -- luacheck: no global
> > +  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
> 
> Let's declare _TARANTOOL as global in .luacheckrc instead of suppressing 
> warning inline every time.
> 
> About 7 tests suppress it.

I suppose it is a good thing to change, but in a separate patchset.
Ignoring for now, you may create a ticket for refactoring if you want
(to prevent falling off the edge of the earth).

> 
> > +})

<snipped>

> > +test:done(true)

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 5/6] FFI: Fix dangling reference to CType. Improve checks.

[Sergey Bronnikov via Tarantool-patches]

Hello,


On 11/30/23 10:25, Sergey Kaplun wrote:


<snipped>

>> failed to reproduce a bug with reverted fix and LuaJIT built with a flag:
>>
>> $ CMAKE_C_FLAGS=-DLUAJIT_CTYPE_CHECK_ANCHOR
>> CFLAGS=-DLUAJIT_CTYPE_CHECK_ANCHOR cmake -S . -B build
>> -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUAJIT_USE_ASAN=ON
> I suppose you forgot the -DLUAJIT_USE_SYSMALLOC=ON
> -DLUAJIT_ENABLE_GC64=ON flags. The same is valid for the previous patch.

It was not obvious that bug is specific for GC64.

LGTM now.

<snipped>

Re: [Tarantool-patches] [PATCH luajit 6/6] FFI: Fix dangling reference to CType in carith_checkarg().

[Sergey Bronnikov via Tarantool-patches]

Thanks for the patch! LGTM

On 10/23/23 12:22, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Sergey Kaplun.
>
> (cherry-picked from commit db944b2b56c86fcf133745976763604d96110285)
>
> During of an arithmetic operation with a cdata function object and some
> cdata value in `carith_checkarg()`, reallocation of `cts->tab` in
> `lj_ctype_intern()` may occur. In that case, the reference to the first
> `CType` object (`ca->ct[0]`) becomes invalid. This patch saves the
> `CTypeID` of this object and gets its `CType` again after possible
> reallocation.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9145
> ---
<snipped>

Re: [Tarantool-patches] [PATCH luajit 0/6] FFI fixes

[Sergey Bronnikov via Tarantool-patches]

LGTM patch series

On 10/23/23 12:22, Sergey Kaplun wrote:
> This patchset is an umbrella for all FFI fixes and refactoring to be
> done in #9145.
>
> The first patch is a refactoring, and it will be useful for us when we
> do #4738. The next two patches are just some FFI bugs to be fixed. The
> last three patches are minor fixes use-after-free references of the
> `cts->tab` objects.
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/ffi-fixes
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9285
> Related issues:
> * https://github.com/LuaJIT/LuaJIT/issues/745
> * https://github.com/LuaJIT/LuaJIT/issues/920
> * https://github.com/LuaJIT/LuaJIT/issues/1108
> * https://github.com/tarantool/tarantool/issues/9145
> * https://github.com/tarantool/tarantool/issues/4738
>
<snipped>

Re: [Tarantool-patches] [PATCH luajit 0/6] FFI fixes

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 23.10.23, Sergey Kaplun via Tarantool-patches wrote:
> This patchset is an umbrella for all FFI fixes and refactoring to be
> done in #9145.
> 
> The first patch is a refactoring, and it will be useful for us when we
> do #4738. The next two patches are just some FFI bugs to be fixed. The
> last three patches are minor fixes use-after-free references of the
> `cts->tab` objects.
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/ffi-fixes
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9285
> Related issues:
> * https://github.com/LuaJIT/LuaJIT/issues/745
> * https://github.com/LuaJIT/LuaJIT/issues/920
> * https://github.com/LuaJIT/LuaJIT/issues/1108
> * https://github.com/tarantool/tarantool/issues/9145
> * https://github.com/tarantool/tarantool/issues/4738
> 
> Mike Pall (6):
>   Abstract out on-demand loading of FFI library.
>   FFI: Fix missing cts->L initialization in argv2ctype().
>   FFI: Ensure returned string is alive in ffi.typeinfo().
>   FFI: Fix dangling reference to CType.
>   FFI: Fix dangling reference to CType. Improve checks.
>   FFI: Fix dangling reference to CType in carith_checkarg().
> 
>  src/lib_ffi.c                                 |  1 +
>  src/lib_jit.c                                 |  6 +-
>  src/lj_bcread.c                               |  6 +-
>  src/lj_carith.c                               |  4 +
>  src/lj_cconv.c                                |  2 +
>  src/lj_crecord.c                              |  6 +-
>  src/lj_ctype.c                                | 12 +++
>  src/lj_ctype.h                                | 10 +++
>  src/lj_lex.c                                  |  6 +-
>  .../fix-argv2ctype-cts-L-init.test.lua        | 15 ++++
>  .../fix-argv2ctype-cts-L-init/script.lua      | 14 ++++
>  .../fix-dangling-reference-to-ctype.test.lua  | 59 ++++++++++++++
>  ...8-fix-dangling-reference-to-ctype.test.lua | 67 ++++++++++++++++
>  .../lj-745-ffi-typeinfo-dead-names.test.lua   | 28 +++++++
>  ...0-fix-dangling-reference-to-ctype.test.lua | 77 +++++++++++++++++++
>  15 files changed, 297 insertions(+), 16 deletions(-)
>  create mode 100644 test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua
>  create mode 100644 test/tarantool-tests/fix-argv2ctype-cts-L-init/script.lua
>  create mode 100644 test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
>  create mode 100644 test/tarantool-tests/lj-1108-fix-dangling-reference-to-ctype.test.lua
>  create mode 100644 test/tarantool-tests/lj-745-ffi-typeinfo-dead-names.test.lua
>  create mode 100644 test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> 
> -- 
> 2.42.0
> 

-- 
Best regards,
IM