From: Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: tarantool-patches@dev.tarantool.org, skaplun@tarantool.org, sergeyb@tarantool.org Subject: [Tarantool-patches] [PATCH luajit] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*. Date: Thu, 21 Sep 2023 16:15:31 +0300 [thread overview] Message-ID: <20230921131539.255389-1-m.kokryashkin@tarantool.org> (raw) From: Mike Pall <mike> Reported by Arseny Vakhrushev. Fix contributed by Peter Cawley. As specified in lj_record.c:304, all loops must set `J->pc` to the next instruction. However, the chunk of logic at lj_trace.c:923 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. Maxim Kokryashkin: * added the description and the test for the problem Part of tarantool/tarantool#8825 --- Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-624-jloop-snapshot-pc PR: https://github.com/tarantool/tarantool/pull/9166 NB: The test for this patch triggers the assertion added in this patch, however I had no luck making a __stable__ reproducer for the issue, since it depends on what's in memory after the BC_JLOOP. It is easier to achieve a consitent failures if ASLR is disabled, but it's not suitable for the testing purposes. src/lj_record.c | 9 +++++---- src/lj_snap.c | 3 +++ .../lj-624-jloop-snapshot-pc.test.lua | 16 ++++++++++++++++ 3 files changed, 24 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/src/lj_snap.c b/src/lj_snap.c index 2dc281cb..b50ecfb2 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)) { 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..ada290ff --- /dev/null +++ b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua @@ -0,0 +1,16 @@ +local tap = require('tap') +local test = tap.test('lj-624-jloop-snapshot-pc'):skipcond({ + ['Test requires JIT enabled'] = not jit.status(), +}) + +test:plan(1) + +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.42.0
next reply other threads:[~2023-09-21 13:15 UTC|newest] Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-09-21 13:15 Maxim Kokryashkin via Tarantool-patches [this message] 2023-09-22 7:51 ` Sergey Kaplun via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230921131539.255389-1-m.kokryashkin@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=max.kokryashkin@gmail.com \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox