Re: [Tarantool-patches] [PATCH luajit] Fix unsinking of IR_FSTORE for NULL metatable.

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

Hi, Sergey

thanks for the patch! LGTM

On 1/31/24 15:18, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by pwnhacker0x18.
>
> (cherry picked from commit 85b4fed0b0353dd78c8c875c2f562d522a2b310f)
>
> The `FSTORE` restoring of a sunk table from a snapshot for
> `IRFL_TAB_META` misses the case when the second argument of
> `setmetatable()` is `nil` (so, the `FSTORE` second operand is `NULL`).
> This may lead to the corresponding assertion failure in the
> `snap_replay_const()` or the crash.
>
> This patch handles the aforementioned case.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9595
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1147-fstore-null-meta
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9635
> Related issues:
> * https://github.com/tarantool/tarantool/issues/9595
> * https://github.com/LuaJIT/LuaJIT/issues/1147
>
>   src/lj_snap.c                                 | 11 +++--
>   .../lj-1147-fstore-null-meta.test.lua         | 41 +++++++++++++++++++
>   2 files changed, 49 insertions(+), 3 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1147-fstore-null-meta.test.lua
>
> diff --git a/src/lj_snap.c b/src/lj_snap.c
> index 73e18e69..26352080 100644
> --- a/src/lj_snap.c
> +++ b/src/lj_snap.c
> @@ -414,6 +414,7 @@ static TRef snap_replay_const(jit_State *J, IRIns *ir)
>     case IR_KNUM: case IR_KINT64:
>       return lj_ir_k64(J, (IROp)ir->o, ir_k64(ir)->u64);
>     case IR_KPTR: return lj_ir_kptr(J, ir_kptr(ir));  /* Continuation. */
> +  case IR_KNULL: return lj_ir_knull(J, irt_type(ir->t));
>     default: lj_assertJ(0, "bad IR constant op %d", ir->o); return TREF_NIL;
>     }
>   }
> @@ -857,9 +858,13 @@ static void snap_unsink(jit_State *J, GCtrace *T, ExitState *ex,
>   	if (irk->o == IR_FREF) {
>   	  switch (irk->op2) {
>   	  case IRFL_TAB_META:
> -	    snap_restoreval(J, T, ex, snapno, rfilt, irs->op2, &tmp);
> -	    /* NOBARRIER: The table is new (marked white). */
> -	    setgcref(t->metatable, obj2gco(tabV(&tmp)));
> +	    if (T->ir[irs->op2].o == IR_KNULL) {
> +	      setgcrefnull(t->metatable);
> +	    } else {
> +	      snap_restoreval(J, T, ex, snapno, rfilt, irs->op2, &tmp);
> +	      /* NOBARRIER: The table is new (marked white). */
> +	      setgcref(t->metatable, obj2gco(tabV(&tmp)));
> +	    }
>   	    break;
>   	  case IRFL_TAB_NOMM:
>   	    /* Negative metamethod cache invalidated by lj_tab_set() below. */
> diff --git a/test/tarantool-tests/lj-1147-fstore-null-meta.test.lua b/test/tarantool-tests/lj-1147-fstore-null-meta.test.lua
> new file mode 100644
> index 00000000..bdf60a26
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1147-fstore-null-meta.test.lua
> @@ -0,0 +1,41 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT's incorrect restoration of a
> +-- table from a snapshot when the `setmetatable()` gets `nil` as
> +-- the second argument.
> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1147.
> +
> +local test = tap.test('lj-1147-fstore-null-meta'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(2)
> +
> +jit.opt.start('hotloop=1')
> +
> +local counter = 0
> +local tab
> +-- XXX: The loop is limited to 3 iterations to compile a trace and
> +-- start to execute it. Use the loop format to see
> +-- the side effects on the restoration from the snapshot.
> +local LOOP_LIMIT = 2
> +while true do
> +  counter = counter + 1
> +  -- Use counter for the content check.
> +  tab = {counter}
> +  -- This emits the following IRs necessary for the assertion
> +  -- failure:
> +  -- | 0003 }+ tab TNEW   #3    #0
> +  -- | ...
> +  -- | 0015    p64 FREF   0003  tab.meta
> +  -- | 0016 }  tab FSTORE 0015  NULL
> +  setmetatable(tab, nil)
> +  -- Emit exit here to be sure that the table will be restored
> +  -- from the snapshot.
> +  if counter > LOOP_LIMIT then break end
> +end
> +
> +test:is(tab[1], LOOP_LIMIT + 1, 'correct table content')
> +test:ok(debug.getmetatable(tab) == nil, 'no metatable on the restored table')
> +
> +test:done(true)