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

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

[Sergey Kaplun via Tarantool-patches]

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)
-- 
2.43.0

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM

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

[Sergey Bronnikov via Tarantool-patches]

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)

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

[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/3.0 and
release/2.11.

On 31.01.24, Sergey Kaplun via Tarantool-patches 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
> 

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
IM