Hello, Sergey!

Thanks for the patch! See a comment below.

Sergey

From: Mike Pall <mike>

Reported by Junlong Li. Fixed by Peter Cawley.

(cherry picked from commit 86e7123bb1782a5f200ba5e83b8c4f3fbad4f7bc)

This patch is a follow-up to the previous commit, which leads to a dirty
read of the pseudo-valid PC set for the cframe on snapshot restoration.
To avoid these dirty reads, this patch sets the PC to the outer frame
as well before possible error throwing.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#11278
---
 src/lj_snap.c                                 |  3 +-
 src/lj_trace.c                                |  4 +-
 ...-1196-stack-overflow-snap-restore.test.lua | 65 +++++++++++++++++++
 3 files changed, 68 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1196-stack-overflow-snap-restore.test.lua

diff --git a/src/lj_snap.c b/src/lj_snap.c
index 8d7bd868..4cfae579 100644
--- a/src/lj_snap.c
+++ b/src/lj_snap.c
@@ -955,7 +955,8 @@ const BCIns *lj_snap_restore(jit_State *J, void *exptr)
   lua_State *L = J->L;
 
   /* Set interpreter PC to the next PC to get correct error messages. */
-  setcframe_pc(cframe_raw(L->cframe), pc+1);
+  setcframe_pc(L->cframe, pc+1);
+  setcframe_pc(cframe_raw(cframe_prev(L->cframe)), pc);
 
   /* Make sure the stack is big enough for the slots from the snapshot. */
   if (LJ_UNLIKELY(L->base + snap->topslot >= tvref(L->maxstack))) {
diff --git a/src/lj_trace.c b/src/lj_trace.c
index 8a18d3cf..0d1d233a 100644
--- a/src/lj_trace.c
+++ b/src/lj_trace.c
@@ -909,10 +909,8 @@ int LJ_FASTCALL lj_trace_exit(jit_State *J, void *exptr)
   exd.J = J;
   exd.exptr = exptr;
   errcode = lj_vm_cpcall(L, NULL, &exd, trace_exit_cp);
-  if (errcode) {
-    setcframe_pc(cframe_raw(L->cframe), L);  /* Point to any valid memory. */
+  if (errcode)
     return -errcode;  /* Return negated error code. */
-  }
 
   if (exitcode) copyTV(L, L->top++, &exiterr);  /* Anchor the error object. */
 
diff --git a/test/tarantool-tests/lj-1196-stack-overflow-snap-restore.test.lua b/test/tarantool-tests/lj-1196-stack-overflow-snap-restore.test.lua
new file mode 100644
index 00000000..942d1f82
--- /dev/null
+++ b/test/tarantool-tests/lj-1196-stack-overflow-snap-restore.test.lua
@@ -0,0 +1,65 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT dirty reads after stack
+-- overflow during restoration from the snapshot.
+-- The test fails before the patch under Valgrind.

Please specify valgrind option that is required for reproducing the bug.

Cannot reproduce with command line below with reverted patch:

VALGRIND_OPTS="--leak-check=no --malloc-fill=0x00 --free-fill=0x00" ctest -V -R test/tarantool-tests/lj-1196-partial-snap-restore.test.lua -V

+--
+-- luacheck: push no max_comment_line_length
+--
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1196,
+-- https://www.freelists.org/post/luajit/Invalid-read-found-by-valgrind.
+--
+-- luacheck: pop
+
+local test = tap.test('lj-1196-stack-overflow-snap-restore')
+
+test:plan(4)
+
+-- XXX: This file has the same tests as the
+-- <test/LuaJIT-tests/lang/stackov.lua>, but without disabling the
+-- compilation for the given functions. Hence, the check here is
+-- less strict -- we just check that there are no dirty reads,
+-- uninitialized memory access, etc.
+
+local function recursive_f_noarg()
+  recursive_f_noarg()
+end
+
+local function recursive_one_arg(argument)
+  recursive_one_arg(argument)
+end
+
+local function recursive_f_vararg(...)
+  recursive_f_vararg(1, ...)
+end
+
+local function recursive_f_vararg_tail(...)
+  return recursive_f_vararg_tail(1, ...)
+end
+
+-- Use `coroutine.wrap()`, for independent stack sizes.
+-- The invalid read is done by the error handler
+-- `debug.traceback()`, since it observes the pseudo PC (`L`) and
+-- reads the memory by `L - 4` address before the patch.
+
+coroutine.wrap(function()
+  local status = xpcall(recursive_f_noarg, debug.traceback)
+  test:ok(not status, 'correct status, recursive no arguments')
+end)()
+
+coroutine.wrap(function()
+  local status = xpcall(recursive_one_arg, debug.traceback, 1)
+  test:ok(not status, 'correct status, recursive one argument')
+end)()
+
+coroutine.wrap(function()
+  local status = xpcall(recursive_f_vararg, debug.traceback, 1)
+  test:ok(not status, 'correct status, recursive vararg')
+end)()
+
+coroutine.wrap(function()
+  local status = xpcall(recursive_f_vararg_tail, debug.traceback, 1)
+  test:ok(not status, 'correct status, recursive vararg tail')
+end)()
+
+test:done(true)