[Tarantool-patches] [PATCH luajit v3 0/2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.

[Tarantool-patches] [PATCH luajit v3 0/2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.

[Maksim Kokryashkin via Tarantool-patches]

Changes in v3:
- Fixed comments as per review by Sergey
- The patch was split into two, so the test case becomes easier to
implement since it can now depend on this assertion instead
of memory layout.

Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-624-jloop-snapshot-pc
PR: https://github.com/tarantool/tarantool/pull/9166
Issue: https://github.com/luajit/luajit/issues/624

Mike Pall (2):
  snap: check J->pc is within its proto bytecode
  Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.

 src/lj_record.c                               |  9 +-
 src/lj_snap.c                                 |  3 +
 .../lj-624-jloop-snapshot-pc.test.lua         | 84 +++++++++++++++++++
 3 files changed, 92 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua

--
2.39.3 (Apple Git-145)

[Tarantool-patches] [PATCH luajit v3 1/2] snap: check J->pc is within its proto bytecode

[Maksim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

(cherry-picked from commit 5c46f47736f7609be407c88d531ecd1689d40a79)

This commit adds an assertion to ensure that the `pc` of the
snapshot being made is located within the current prototype.
Violation of this assertion's condition may lead to all kinds
of buggy behavior on restoration from that snapshot, depending
on what is located in memory at the address under `pc`.

NOTICE: This patch is only a part of the original commit,
and the other part is backported in the following commit. The
patch was split into two, so the test case becomes easier to
implement since it can now depend on this assertion instead
of memory layout.

Maxim Kokryashkin:
* added the description for the problem

Part of tarantool/tarantool#9145
---
 src/lj_snap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/lj_snap.c b/src/lj_snap.c
index 6c5e5e53..3f0fccec 100644
--- a/src/lj_snap.c
+++ b/src/lj_snap.c
@@ -115,6 +115,9 @@ static MSize snapshot_framelinks(jit_State *J, SnapEntry *map, uint8_t *topslot)
 #else
   MSize f = 0;
   map[f++] = SNAP_MKPC(J->pc);  /* The current PC is always the first entry. */
+  lj_assertJ(!J->pt ||
+	     (J->pc >= proto_bc(J->pt) &&
+	      J->pc < proto_bc(J->pt) + J->pt->sizebc), "bad snapshot PC");
 #endif
   while (frame > lim) {  /* Backwards traversal of all frames above base. */
     if (frame_islua(frame)) {
-- 
2.39.3 (Apple Git-145)

Re: [Tarantool-patches] [PATCH luajit v3 1/2] snap: check J->pc is within its proto bytecode

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v3 1/2] snap: check J->pc is within its proto bytecode

[Sergey Bronnikov via Tarantool-patches]

Hi, Maxim


LGTM

On 10/4/23 15:50, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> (cherry-picked from commit 5c46f47736f7609be407c88d531ecd1689d40a79)
>
> This commit adds an assertion to ensure that the `pc` of the
> snapshot being made is located within the current prototype.
> Violation of this assertion's condition may lead to all kinds
> of buggy behavior on restoration from that snapshot, depending
> on what is located in memory at the address under `pc`.
>
> NOTICE: This patch is only a part of the original commit,
> and the other part is backported in the following commit. The
> patch was split into two, so the test case becomes easier to
> implement since it can now depend on this assertion instead
> of memory layout.
>
> Maxim Kokryashkin:
> * added the description for the problem
>
> Part of tarantool/tarantool#9145
> ---
>   src/lj_snap.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/src/lj_snap.c b/src/lj_snap.c
> index 6c5e5e53..3f0fccec 100644
> --- a/src/lj_snap.c
> +++ b/src/lj_snap.c
> @@ -115,6 +115,9 @@ static MSize snapshot_framelinks(jit_State *J, SnapEntry *map, uint8_t *topslot)
>   #else
>     MSize f = 0;
>     map[f++] = SNAP_MKPC(J->pc);  /* The current PC is always the first entry. */
> +  lj_assertJ(!J->pt ||
> +	     (J->pc >= proto_bc(J->pt) &&
> +	      J->pc < proto_bc(J->pt) + J->pt->sizebc), "bad snapshot PC");
>   #endif
>     while (frame > lim) {  /* Backwards traversal of all frames above base. */
>       if (frame_islua(frame)) {

[Tarantool-patches] [PATCH luajit v3 2/2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.

[Maksim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

Reported by Arseny Vakhrushev.
Fix contributed by Peter Cawley.

(cherry-picked from commit 5c46f47736f7609be407c88d531ecd1689d40a79)

As specified in the comment in `lj_record_stop`, all loops must
set `J->pc` to the next instruction. However, the chunk of logic
in `lj_trace_exit` expects it to be set to `BC_JLOOP` itself if
it used to be a `BC_RET`. This wrong pc results in the execution
of random data that goes after `BC_JLOOP` in the case of
restoration from the snapshot.

This patch fixes that behavior by adapting the loop recording
logic to this specific case.

NOTICE: This patch is only a part of the original commit,
and the other part is backported in the previous commit. The
patch was split into two, so the test case becomes easier to
implement since it can now depend on this assertion instead
of memory layout.

Maxim Kokryashkin:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
 src/lj_record.c                               |  9 +-
 .../lj-624-jloop-snapshot-pc.test.lua         | 84 +++++++++++++++++++
 2 files changed, 89 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua

diff --git a/src/lj_record.c b/src/lj_record.c
index 48a5481b..3bdc6134 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -570,10 +570,10 @@ static LoopEvent rec_iterl(jit_State *J, const BCIns iterins)
 }
 
 /* Record LOOP/JLOOP. Now, that was easy. */
-static LoopEvent rec_loop(jit_State *J, BCReg ra)
+static LoopEvent rec_loop(jit_State *J, BCReg ra, int skip)
 {
   if (ra < J->maxslot) J->maxslot = ra;
-  J->pc++;
+  J->pc += skip;
   return LOOPEV_ENTER;
 }
 
@@ -2433,7 +2433,7 @@ void lj_record_ins(jit_State *J)
     rec_loop_interp(J, pc, rec_iterl(J, *pc));
     break;
   case BC_LOOP:
-    rec_loop_interp(J, pc, rec_loop(J, ra));
+    rec_loop_interp(J, pc, rec_loop(J, ra, 1));
     break;
 
   case BC_JFORL:
@@ -2443,7 +2443,8 @@ void lj_record_ins(jit_State *J)
     rec_loop_jit(J, rc, rec_iterl(J, traceref(J, rc)->startins));
     break;
   case BC_JLOOP:
-    rec_loop_jit(J, rc, rec_loop(J, ra));
+    rec_loop_jit(J, rc, rec_loop(J, ra,
+				 !bc_isret(bc_op(traceref(J, rc)->startins))));
     break;
 
   case BC_IFORL:
diff --git a/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
new file mode 100644
index 00000000..726b2efa
--- /dev/null
+++ b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
@@ -0,0 +1,84 @@
+local tap = require('tap')
+local test = tap.test('lj-624-jloop-snapshot-pc'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+-- XXX: The test case below triggers the assertion that was
+-- added in the patch if tested without the fix itself. It
+-- is hard to create a stable reproducer without turning off
+-- ASLR and VM randomizations, which is not suitable for testing.
+
+-- Reproducer below produces the following traces:
+-- ---- TRACE 1 start test.lua:2
+-- 0001  KSHORT   1   2
+-- 0002  ISGE     0   1
+-- 0003  JMP      1 => 0006
+-- 0006  UGET     1   0      ; fib
+-- 0007  SUBVN    2   0   0  ; 1
+-- 0008  CALL     1   2   2
+-- 0000  . FUNCF    4          ; test.lua:2
+-- 0001  . KSHORT   1   2
+-- 0002  . ISGE     0   1
+-- 0003  . JMP      1 => 0006
+-- 0006  . UGET     1   0      ; fib
+-- 0007  . SUBVN    2   0   0  ; 1
+-- 0008  . CALL     1   2   2
+-- 0000  . . FUNCF    4          ; test.lua:2
+-- 0001  . . KSHORT   1   2
+-- 0002  . . ISGE     0   1
+-- 0003  . . JMP      1 => 0006
+-- 0006  . . UGET     1   0      ; fib
+-- 0007  . . SUBVN    2   0   0  ; 1
+-- 0008  . . CALL     1   2   2
+-- 0000  . . . FUNCF    4          ; test.lua:2
+-- ---- TRACE 1 stop -> up-recursion
+--
+-- ---- TRACE 1 exit 1
+-- ---- TRACE 2 start 1/1 test.lua:3
+-- 0004  ISTC     1   0
+-- 0005  JMP      1 => 0013
+-- 0013  RET1     1   2
+-- 0009  UGET     2   0      ; fib
+-- 0010  SUBVN    3   0   1  ; 2
+-- 0011  CALL     2   2   2
+-- 0000  . JFUNCF   4   1         ; test.lua:2
+-- ---- TRACE 2 stop -> 1
+--
+-- ---- TRACE 2 exit 1
+-- ---- TRACE 3 start 2/1 test.lua:3
+-- 0013  RET1     1   2
+-- 0012  ADDVV    1   1   2
+-- 0013  RET1     1   2
+-- ---- TRACE 3 abort test.lua:3 -- down-recursion, restarting
+--
+-- ---- TRACE 3 start test.lua:3
+-- 0013  RET1     1   2
+-- 0009  UGET     2   0      ; fib
+-- 0010  SUBVN    3   0   1  ; 2
+-- 0011  CALL     2   2   2
+-- 0000  . JFUNCF   4   1         ; test.lua:2
+-- ---- TRACE 3 stop -> 1
+--
+-- ---- TRACE 2 exit 1
+-- ---- TRACE 4 start 2/1 test.lua:3
+-- 0013  RET1     1   2
+-- 0012  ADDVV    1   1   2
+-- 0013  JLOOP    3   3
+--
+-- During the recording of the latter JLOOP the assertion added
+-- in the patch is triggered.
+--
+-- See also:
+-- https://github.com/luaJIT/LuaJIT/issues/624
+
+
+jit.opt.start('hotloop=1', 'hotexit=1')
+local function fib(n)
+  return n < 2 and n or fib(n - 1) + fib(n - 2)
+end
+
+fib(5)
+
+test:ok(true, 'snapshot pc is correct')
+test:done(true)
-- 
2.39.3 (Apple Git-145)

Re: [Tarantool-patches] [PATCH luajit v3 2/2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.

[Sergey Kaplun via Tarantool-patches]

Hi, Maksim!
Thanks for the patch!
LGTM, just a few nits regarding to the comments in the test.

On 04.10.23, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
> 

<snipped>

> diff --git a/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua

<snipped>

> +--
> +-- ---- TRACE 2 exit 1
> +-- ---- TRACE 4 start 2/1 test.lua:3
> +-- 0013  RET1     1   2
> +-- 0012  ADDVV    1   1   2
> +-- 0013  JLOOP    3   3
> +--
> +-- During the recording of the latter JLOOP the assertion added
> +-- in the patch is triggered.

Minor: I suggest to rephrase this as the following:

| The assertion introduced in the previous patch is triggered during
| recording of the last 0013 JLOOP.

> +--
> +-- See also:
> +-- https://github.com/luaJIT/LuaJIT/issues/624
> +
> +

Nit: extra empty line.

> +jit.opt.start('hotloop=1', 'hotexit=1')
> +local function fib(n)
> +  return n < 2 and n or fib(n - 1) + fib(n - 2)
> +end
> +
> +fib(5)
> +
> +test:ok(true, 'snapshot pc is correct')
> +test:done(true)
> -- 
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v3 2/2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Fixed your comments, the branch is force-pushed.

You can find the diff below.
On Tue, Oct 10, 2023 at 11:14:36AM +0300, Sergey Kaplun wrote:
> Hi, Maksim!
> Thanks for the patch!
> LGTM, just a few nits regarding to the comments in the test.
> 
> On 04.10.23, Maksim Kokryashkin wrote:
> > From: Mike Pall <mike>
> > 
> 
> <snipped>
> 
> > diff --git a/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
> 
> <snipped>
> 
> > +--
> > +-- ---- TRACE 2 exit 1
> > +-- ---- TRACE 4 start 2/1 test.lua:3
> > +-- 0013  RET1     1   2
> > +-- 0012  ADDVV    1   1   2
> > +-- 0013  JLOOP    3   3
> > +--
> > +-- During the recording of the latter JLOOP the assertion added
> > +-- in the patch is triggered.
> 
> Minor: I suggest to rephrase this as the following:
> 
> | The assertion introduced in the previous patch is triggered during
> | recording of the last 0013 JLOOP.
> 
> > +--
> > +-- See also:
> > +-- https://github.com/luaJIT/LuaJIT/issues/624
> > +
> > +
> 
> Nit: extra empty line.
Diff with changes:
===
diff --git a/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
index 726b2efa..565e4cbf 100644
--- a/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
+++ b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
@@ -66,13 +66,12 @@ test:plan(1)
 -- 0012  ADDVV    1   1   2
 -- 0013  JLOOP    3   3
 --
--- During the recording of the latter JLOOP the assertion added
--- in the patch is triggered.
+-- The assertion introduced in the previous patch is triggered during
+-- recording of the last 0013 JLOOP.
 --
 -- See also:
 -- https://github.com/luaJIT/LuaJIT/issues/624
 
-
 jit.opt.start('hotloop=1', 'hotexit=1')
 local function fib(n)
   return n < 2 and n or fib(n - 1) + fib(n - 2)
===

> 
> > +jit.opt.start('hotloop=1', 'hotexit=1')
> > +local function fib(n)
> > +  return n < 2 and n or fib(n - 1) + fib(n - 2)
> > +end
> > +
> > +fib(5)
> > +
> > +test:ok(true, 'snapshot pc is correct')
> > +test:done(true)
> > -- 
> > 2.39.3 (Apple Git-145)
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v3 2/2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.

[Sergey Bronnikov via Tarantool-patches]

Hi, Maxim

LGTM with a minor comment

On 10/4/23 15:50, Maksim Kokryashkin wrote:


<snipped>

> --- /dev/null
> +++ b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
> @@ -0,0 +1,84 @@
> +local tap = require('tap')
> +local test = tap.test('lj-624-jloop-snapshot-pc'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +-- XXX: The test case below triggers the assertion that was
> +-- added in the patch if tested without the fix itself. It
> +-- is hard to create a stable reproducer without turning off
> +-- ASLR and VM randomizations, which is not suitable for testing.
Probably it would be useful to say it in commit message.


<snipped>

Re: [Tarantool-patches] [PATCH luajit v3 0/2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.

[Igor Munkin via Tarantool-patches]

Max,

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 04.10.23, Maksim Kokryashkin via Tarantool-patches wrote:
> Changes in v3:
> - Fixed comments as per review by Sergey
> - The patch was split into two, so the test case becomes easier to
> implement since it can now depend on this assertion instead
> of memory layout.
> 
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-624-jloop-snapshot-pc
> PR: https://github.com/tarantool/tarantool/pull/9166
> Issue: https://github.com/luajit/luajit/issues/624
> 
> Mike Pall (2):
>   snap: check J->pc is within its proto bytecode
>   Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.
> 
>  src/lj_record.c                               |  9 +-
>  src/lj_snap.c                                 |  3 +
>  .../lj-624-jloop-snapshot-pc.test.lua         | 84 +++++++++++++++++++
>  3 files changed, 92 insertions(+), 4 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
> 
> --
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
IM