Re: [Tarantool-patches] [PATCH luajit 1/2] FFI: Fix dangling CType references.

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

Hi, Sergey,

thanks for the patch!

LGTM

Sergey

On 8/22/25 09:36, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Sergey Kaplun.
>
> (cherry picked from commit 9c8eb7cfe10ef5939d9b358a0bd805a610818ba5)
>
> In case when the `ccall_set_args()` (and `crec_call_args()` in JIT)
> reallocates the ctype table, the references of the `ct` become invalid,
> and `ct->info` dereferences the invalid pointer.
>
> This patch fixes this by preserving the ctype info and ctype id to avoid
> dereferencing invalid pointer.
>
> This patch doesn't properly fix the issue for the JIT recording, since
> the child ctype reference is still in use. It will be fixed in the next
> patch.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#11691
> ---
>   src/lj_ccall.c                                | 19 +++--
>   src/lj_crecord.c                              | 21 +++---
>   ...-1360-dangling-ctype-ref-on-ccall.test.lua | 70 +++++++++++++++++++
>   3 files changed, 95 insertions(+), 15 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1360-dangling-ctype-ref-on-ccall.test.lua
>
> diff --git a/src/lj_ccall.c b/src/lj_ccall.c
> index a989f657..c3b27572 100644
> --- a/src/lj_ccall.c
> +++ b/src/lj_ccall.c
> @@ -890,7 +890,9 @@ void ccall_copy_struct(CCallState *cc, CType *ctr, void *dp, void *sp, int ft)
>   
>   /* -- Common C call handling ---------------------------------------------- */
>   
> -/* Infer the destination CTypeID for a vararg argument. */
> +/* Infer the destination CTypeID for a vararg argument.
> +** Note: may reallocate cts->tab and invalidate CType pointers.
> +*/
>   CTypeID lj_ccall_ctid_vararg(CTState *cts, cTValue *o)
>   {
>     if (tvisnumber(o)) {
> @@ -918,13 +920,16 @@ CTypeID lj_ccall_ctid_vararg(CTState *cts, cTValue *o)
>     }
>   }
>   
> -/* Setup arguments for C call. */
> +/* Setup arguments for C call.
> +** Note: may reallocate cts->tab and invalidate CType pointers.
> +*/
>   static int ccall_set_args(lua_State *L, CTState *cts, CType *ct,
>   			  CCallState *cc)
>   {
>     int gcsteps = 0;
>     TValue *o, *top = L->top;
>     CTypeID fid;
> +  CTInfo info = ct->info;  /* lj_ccall_ctid_vararg may invalidate ct pointer. */
>     CType *ctr;
>     MSize maxgpr, ngpr = 0, nsp = 0, narg;
>   #if CCALL_NARG_FPR
> @@ -943,7 +948,7 @@ static int ccall_set_args(lua_State *L, CTState *cts, CType *ct,
>   #if LJ_TARGET_X86
>     /* x86 has several different calling conventions. */
>     cc->resx87 = 0;
> -  switch (ctype_cconv(ct->info)) {
> +  switch (ctype_cconv(info)) {
>     case CTCC_FASTCALL: maxgpr = 2; break;
>     case CTCC_THISCALL: maxgpr = 1; break;
>     default: maxgpr = 0; break;
> @@ -960,7 +965,7 @@ static int ccall_set_args(lua_State *L, CTState *cts, CType *ct,
>     } else if (ctype_iscomplex(ctr->info) || ctype_isstruct(ctr->info)) {
>       /* Preallocate cdata object and anchor it after arguments. */
>       CTSize sz = ctr->size;
> -    GCcdata *cd = lj_cdata_new(cts, ctype_cid(ct->info), sz);
> +    GCcdata *cd = lj_cdata_new(cts, ctype_cid(info), sz);
>       void *dp = cdataptr(cd);
>       setcdataV(L, L->top++, cd);
>       if (ctype_isstruct(ctr->info)) {
> @@ -996,7 +1001,7 @@ static int ccall_set_args(lua_State *L, CTState *cts, CType *ct,
>         lj_assertL(ctype_isfield(ctf->info), "field expected");
>         did = ctype_cid(ctf->info);
>       } else {
> -      if (!(ct->info & CTF_VARARG))
> +      if (!(info & CTF_VARARG))
>   	lj_err_caller(L, LJ_ERR_FFI_NUMARG);  /* Too many arguments. */
>         did = lj_ccall_ctid_vararg(cts, o);  /* Infer vararg type. */
>         isva = 1;
> @@ -1157,11 +1162,11 @@ int lj_ccall_func(lua_State *L, GCcdata *cd)
>       ct = ctype_rawchild(cts, ct);
>     }
>     if (ctype_isfunc(ct->info)) {
> +    CTypeID id = ctype_typeid(cts, ct);
>       CCallState cc;
>       int gcsteps, ret;
>       cc.func = (void (*)(void))cdata_getptr(cdataptr(cd), sz);
>       gcsteps = ccall_set_args(L, cts, ct, &cc);
> -    ct = (CType *)((intptr_t)ct-(intptr_t)cts->tab);
>       cts->cb.slot = ~0u;
>       lj_vm_ffi_call(&cc);
>       if (cts->cb.slot != ~0u) {  /* Blacklist function that called a callback. */
> @@ -1169,7 +1174,7 @@ int lj_ccall_func(lua_State *L, GCcdata *cd)
>         tv.u64 = ((uintptr_t)(void *)cc.func >> 2) | U64x(800000000, 00000000);
>         setboolV(lj_tab_set(L, cts->miscmap, &tv), 1);
>       }
> -    ct = (CType *)((intptr_t)ct+(intptr_t)cts->tab);  /* May be reallocated. */
> +    ct = ctype_get(cts, id);  /* Table may have been reallocated. */
>       gcsteps += ccall_get_results(L, cts, ct, &cc, &ret);
>   #if LJ_TARGET_X86 && LJ_ABI_WIN
>       /* Automatically detect __stdcall and fix up C function declaration. */
> diff --git a/src/lj_crecord.c b/src/lj_crecord.c
> index dcb90216..935106dc 100644
> --- a/src/lj_crecord.c
> +++ b/src/lj_crecord.c
> @@ -1099,12 +1099,15 @@ static void crec_alloc(jit_State *J, RecordFFData *rd, CTypeID id)
>       crec_finalizer(J, trcd, 0, fin);
>   }
>   
> -/* Record argument conversions. */
> +/* Record argument conversions.
> +** Note: may reallocate cts->tab and invalidate CType pointers.
> +*/
>   static TRef crec_call_args(jit_State *J, RecordFFData *rd,
>   			   CTState *cts, CType *ct)
>   {
>     TRef args[CCI_NARGS_MAX];
>     CTypeID fid;
> +  CTInfo info = ct->info;  /* lj_ccall_ctid_vararg may invalidate ct pointer. */
>     MSize i, n;
>     TRef tr, *base;
>     cTValue *o;
> @@ -1113,9 +1116,9 @@ static TRef crec_call_args(jit_State *J, RecordFFData *rd,
>     TRef *arg0 = NULL, *arg1 = NULL;
>   #endif
>     int ngpr = 0;
> -  if (ctype_cconv(ct->info) == CTCC_THISCALL)
> +  if (ctype_cconv(info) == CTCC_THISCALL)
>       ngpr = 1;
> -  else if (ctype_cconv(ct->info) == CTCC_FASTCALL)
> +  else if (ctype_cconv(info) == CTCC_FASTCALL)
>       ngpr = 2;
>   #endif
>   
> @@ -1140,7 +1143,7 @@ static TRef crec_call_args(jit_State *J, RecordFFData *rd,
>         lj_assertJ(ctype_isfield(ctf->info), "field expected");
>         did = ctype_cid(ctf->info);
>       } else {
> -      if (!(ct->info & CTF_VARARG))
> +      if (!(info & CTF_VARARG))
>   	lj_trace_err(J, LJ_TRERR_NYICALL);  /* Too many arguments. */
>         did = lj_ccall_ctid_vararg(cts, o);  /* Infer vararg type. */
>       }
> @@ -1223,12 +1226,14 @@ static int crec_call(jit_State *J, RecordFFData *rd, GCcdata *cd)
>   {
>     CTState *cts = ctype_ctsG(J2G(J));
>     CType *ct = ctype_raw(cts, cd->ctypeid);
> +  CTInfo info;
>     IRType tp = IRT_PTR;
>     if (ctype_isptr(ct->info)) {
>       tp = (LJ_64 && ct->size == 8) ? IRT_P64 : IRT_P32;
>       ct = ctype_rawchild(cts, ct);
>     }
> -  if (ctype_isfunc(ct->info)) {
> +  info = ct->info;  /* crec_call_args may invalidate ct pointer. */
> +  if (ctype_isfunc(info)) {
>       TRef func = emitir(IRT(IR_FLOAD, tp), J->base[0], IRFL_CDATA_PTR);
>       CType *ctr = ctype_rawchild(cts, ct);
>       IRType t = crec_ct2irt(cts, ctr);
> @@ -1245,9 +1250,9 @@ static int crec_call(jit_State *J, RecordFFData *rd, GCcdata *cd)
>   		 ctype_isenum(ctr->info)) || t == IRT_CDATA) {
>         lj_trace_err(J, LJ_TRERR_NYICALL);
>       }
> -    if ((ct->info & CTF_VARARG)
> +    if ((info & CTF_VARARG)
>   #if LJ_TARGET_X86
> -	|| ctype_cconv(ct->info) != CTCC_CDECL
> +	|| ctype_cconv(info) != CTCC_CDECL
>   #endif
>   	)
>         func = emitir(IRT(IR_CARG, IRT_NIL), func,
> @@ -1270,7 +1275,7 @@ static int crec_call(jit_State *J, RecordFFData *rd, GCcdata *cd)
>         }
>       } else if (t == IRT_PTR || (LJ_64 && t == IRT_P32) ||
>   	       t == IRT_I64 || t == IRT_U64 || ctype_isenum(ctr->info)) {
> -      TRef trid = lj_ir_kint(J, ctype_cid(ct->info));
> +      TRef trid = lj_ir_kint(J, ctype_cid(info));
>         tr = emitir(IRTG(IR_CNEWI, IRT_CDATA), trid, tr);
>         if (t == IRT_I64 || t == IRT_U64) lj_needsplit(J);
>       } else if (t == IRT_FLOAT || t == IRT_U32) {
> diff --git a/test/tarantool-tests/lj-1360-dangling-ctype-ref-on-ccall.test.lua b/test/tarantool-tests/lj-1360-dangling-ctype-ref-on-ccall.test.lua
> new file mode 100644
> index 00000000..2d790e43
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1360-dangling-ctype-ref-on-ccall.test.lua
> @@ -0,0 +1,70 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +
> +-- This test demonstrates LuaJIT's incorrect behaviour when the
> +-- reallocation of `cts->tab` strikes during the setup arguments
> +-- for the FFI call.
> +-- Before the patch, the test failed only under ASAN.
> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/1360.
> +local test = tap.test('lj-1360-dangling-ctype-ref-on-ccall'):skipcond({
> +  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
> +})
> +
> +test:plan(1)
> +
> +-- XXX: Declare the structure to increase `cts->top` up to 128
> +-- slots. The setting up of function's arguments to the next call
> +-- will reallocate the `cts->tab` during `lj_ccall_ctid_vararg()`
> +-- and `ccall_set_args()` will use a dangling reference.
> +ffi.cdef[[
> +  struct test {
> +      int a;
> +      int b;
> +      int c;
> +      int d;
> +      int e;
> +      int f;
> +      int g;
> +      int h;
> +      int i;
> +      int j;
> +      int k;
> +      int l;
> +      int m;
> +      int n;
> +      int o;
> +      int p;
> +      int q;
> +      int r;
> +      int s;
> +      int t;
> +      int u;
> +      int v;
> +      int w;
> +      int x;
> +      int y;
> +      int z;
> +      int aa;
> +      int ab;
> +      int ac;
> +      int ad;
> +  };
> +  // Use an existing function that actually takes no arguments.
> +  // We can declare it however we want.
> +  // Need a vararg function for this issue.
> +  int getppid(...);
> +]]
> +
> +local arg_call = ffi.new('struct test')
> +
> +-- Assertions to check the `cts->top` value.
> +assert(ffi.typeinfo(127), 'cts->top >= 127')
> +assert(not ffi.typeinfo(128), 'cts->top < 128')
> +
> +-- Don't check the result, just check that there is no invalid
> +-- memory access.
> +ffi.C.getppid(arg_call)
> +
> +test:ok(true, 'no heap-use-after-free in C call')
> +
> +test:done(true)

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