[Tarantool-patches] [PATCH luajit 1/3][v3] MIPS64: Fix xpcall() error case.

[Tarantool-patches] [PATCH luajit 1/3][v3] MIPS64: Fix xpcall() error case.

[Sergey Bronnikov via Tarantool-patches]

From: Mike Pall <mike>

Thanks to François Perrad and Stefan Pejic.

(cherry picked from commit ea7071d3c30b6432bfe6f8a9d263e0285cec25e3)

The patch fixes `xpcall()` segfaults on MIPS64 architecture.
The similar patch for ARM64 has been backported (with the test)
previously, see the commit af889e4608e6eca495dd85e6161d8bcd7d3628e6
("ARM64: Fix xpcall() error case (really).").

Sergey Bronnikov:
* added the description

Part of tarantool/tarantool#12134
---
 src/vm_mips64.dasc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
index 44fba36c..da187a7a 100644
--- a/src/vm_mips64.dasc
+++ b/src/vm_mips64.dasc
@@ -1440,15 +1440,16 @@ static void build_subroutines(BuildCtx *ctx)
   |.  nop
   |
   |.ffunc xpcall
-  |  daddiu NARGS8:RC, NARGS8:RC, -16
+  |  daddiu NARGS8:TMP0, NARGS8:RC, -16
   |  ld CARG1, 0(BASE)
   |   ld CARG2, 8(BASE)
-  |    bltz NARGS8:RC, ->fff_fallback
+  |    bltz NARGS8:TMP0, ->fff_fallback
   |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)
   |  gettp AT, CARG2
   |  daddiu AT, AT, -LJ_TFUNC
   |  bnez AT, ->fff_fallback		// Traceback must be a function.
   |.   move TMP2, BASE
+  |  move NARGS8:RC, NARGS8:TMP0
   |   daddiu BASE, BASE, 24
   |  // Remember active hook before pcall.
   |  srl TMP3, TMP3, HOOK_ACTIVE_SHIFT
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 2/3][v3] LJ_FR2: Fix stack checks in vararg calls.

[Sergey Bronnikov via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry picked from commit d1a2fef8a8f53b0055ee041f7f63d83a27444ffa)

Stack overflow can cause a segmentation fault in a vararg
function on ARM64 and MIPS64 in LJ_FR2 mode. This happens
because the stack check in BC_IFUNCV is off by one on these
platforms without the patch. The original stack check
for ARM64 and MIPS64 was incorrect:

| RA == BASE + (RD=NARGS)*8 + framesize * 8 >= maxstack

while the stack check on x86_64 is correct and therefore is
not affected by the problem:

| RA == BASE + (RD=NARGS+1)*8 + framesize * 8 +8 > maxstack

The patch partially fixes the aforementioned issue by bumping
LJ_STACK_EXTRA by 1 to give a space to the entire frame link for a
vararg function as the __newindex metamethod.

A fixup for a number of required slots in `call_init()` was added
for consistency with non-GC64 flavor. The check is too strict, so
this can't lead to any crash.

This patch also corrects the number of redzone slots in
luajit-gdb.py to match the updated LJ_STACK_EXTRA and adds the test
<gh-1402-call_init-regression.test.lua> that will help to avoid
a regression in the future, see details in [1].

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

Part of tarantool/tarantool#12134

1. https://github.com/LuaJIT/LuaJIT/issues/1402
---
 src/lj_def.h                                  |  2 +-
 src/lj_dispatch.c                             |  2 +-
 src/luajit-gdb.py                             |  2 +-
 src/vm_arm64.dasc                             |  1 +
 src/vm_mips64.dasc                            |  1 +
 .../gh-1402-call_init-regression.test.lua     | 36 +++++++++++++
 ...048-fix-stack-checks-vararg-calls.test.lua | 53 +++++++++++++++++++
 7 files changed, 94 insertions(+), 3 deletions(-)
 create mode 100644 test/tarantool-tests/gh-1402-call_init-regression.test.lua
 create mode 100644 test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua

diff --git a/src/lj_def.h b/src/lj_def.h
index a5bca6b0..7e4f251e 100644
--- a/src/lj_def.h
+++ b/src/lj_def.h
@@ -69,7 +69,7 @@ typedef unsigned int uintptr_t;
 #define LJ_MAX_UPVAL	60		/* Max. # of upvalues. */
 
 #define LJ_MAX_IDXCHAIN	100		/* __index/__newindex chain limit. */
-#define LJ_STACK_EXTRA	(5+2*LJ_FR2)	/* Extra stack space (metamethods). */
+#define LJ_STACK_EXTRA	(5+3*LJ_FR2)	/* Extra stack space (metamethods). */
 
 #define LJ_NUM_CBPAGE	1		/* Number of FFI callback pages. */
 
diff --git a/src/lj_dispatch.c b/src/lj_dispatch.c
index a44a5adf..431cb3c2 100644
--- a/src/lj_dispatch.c
+++ b/src/lj_dispatch.c
@@ -453,7 +453,7 @@ static int call_init(lua_State *L, GCfunc *fn)
     int numparams = pt->numparams;
     int gotparams = (int)(L->top - L->base);
     int need = pt->framesize;
-    if ((pt->flags & PROTO_VARARG)) need += 1+gotparams;
+    if ((pt->flags & PROTO_VARARG)) need += 1+LJ_FR2+gotparams;
     lj_state_checkstack(L, (MSize)need);
     numparams -= gotparams;
     return numparams >= 0 ? numparams : 0;
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index 0ae2a6e0..dab07b35 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -542,7 +542,7 @@ def dump_stack(L, base=None, top=None):
     top = top or L['top']
     stack = mref('TValue *', L['stack'])
     maxstack = mref('TValue *', L['maxstack'])
-    red = 5 + 2 * LJ_FR2
+    red = 5 + 3 * LJ_FR2
 
     dump = [
         '{padding} Red zone: {nredslots: >2} slots {padding}'.format(
diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
index 6600e226..5ef37243 100644
--- a/src/vm_arm64.dasc
+++ b/src/vm_arm64.dasc
@@ -3779,6 +3779,7 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |   add TMP2, BASE, RC
     |   add LFUNC:CARG3, CARG3, TMP0, lsl #47
     |  add RA, RA, RC
+    |  sub CARG1, CARG1, #8
     |   add TMP0, RC, #16+FRAME_VARG
     |   str LFUNC:CARG3, [TMP2], #8	// Store (tagged) copy of LFUNC.
     |    ldr KBASE, [PC, #-4+PC2PROTO(k)]
diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
index da187a7a..6c2975b4 100644
--- a/src/vm_mips64.dasc
+++ b/src/vm_mips64.dasc
@@ -5268,6 +5268,7 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |   settp LFUNC:RB, TMP0
     |  daddu TMP0, RA, RC
     |   sd LFUNC:RB, 0(TMP1)		// Store (tagged) copy of LFUNC.
+    |  daddiu TMP2, TMP2, -8
     |   daddiu TMP3, RC, 16+FRAME_VARG
     |  sltu AT, TMP0, TMP2
     |    ld KBASE, -4+PC2PROTO(k)(PC)
diff --git a/test/tarantool-tests/gh-1402-call_init-regression.test.lua b/test/tarantool-tests/gh-1402-call_init-regression.test.lua
new file mode 100644
index 00000000..b20f9e39
--- /dev/null
+++ b/test/tarantool-tests/gh-1402-call_init-regression.test.lua
@@ -0,0 +1,36 @@
+local tap = require('tap')
+
+-- A test file to demonstrate a probably quite strict stack
+-- check for vararg functions in call_init.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1402
+local test = tap.test('gh-1402-call_init-regression.test.lua'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+local function vararg(...) -- luacheck: no unused
+  -- None.
+end
+
+-- Make compilation aggressive.
+jit.opt.start("hotloop=1")
+
+local function caller()
+  -- luacheck: push no unused
+  local _, _, _, _, _, _, _, _, _, _
+  local _, _, _, _, _, _, _, _, _, _
+  local _, _, _, _, _, _, _, _, _, _
+  -- luacheck: pop
+  local n = 1
+  while n < 3 do
+    vararg()
+    n = n + 1
+  end
+end
+
+pcall(coroutine.wrap(caller))
+
+test:ok(true, 'no assertion for vararg functions in call_init')
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
new file mode 100644
index 00000000..3a8ad63d
--- /dev/null
+++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
@@ -0,0 +1,53 @@
+local tap = require('tap')
+
+-- A test file to demonstrate a crash due to Lua stack
+-- out-of-bounds access, see below testcase descriptions.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1048.
+local test = tap.test('lj-1048-fix-stack-checks-vararg-calls')
+
+test:plan(2)
+
+-- The test case demonstrates a segmentation fault due to stack
+-- overflow by recursive calling `pcall()`. The functions are
+-- vararg because the stack check in BC_IFUNCV is off by one on
+-- ARM64 and MIPS64 without the patch.
+local function prober_1(...) -- luacheck: no unused
+  -- Any fast function can be used as metamethod, but `type` is
+  -- convenient here because it works fast and can be used with
+  -- any data type. Lua function cannot be used since it
+  -- will check the stack on each invocation. We need to check
+  -- using of the correct value LJ_STACK_EXTRA slots
+  -- (5+3*LJ_FR2) = 8 for GC64 mode.
+  pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, type, 0)
+end
+
+local function looper(prober, n, ...)
+  prober(...)
+  return looper(prober, n + 1, n, ...)
+end
+
+pcall(coroutine.wrap(looper), prober_1, 0)
+
+test:ok(true, 'no stack overflow with recursive pcall')
+
+-- The testcase demonstrate a segmentation fault due to stack
+-- overflow when `pcall()` is used as `__newindex` metamethod.
+-- The function is vararg because stack check in BC_IFUNCV is off
+-- by one on ARM64 and MIPS64 without the patch.
+
+-- Any fast function can be used as metamethod, but `type` is
+-- convenient here because it works fast and can be used with
+-- any data type. Lua function cannot be used since it
+-- will check the stack on each invocation.
+local t = setmetatable({}, { __newindex = pcall, __call = type })
+
+local function prober_2(...) -- luacheck: no unused
+  -- Invokes `pcall(t, t, t)`.
+  t[t] = t
+end
+
+pcall(coroutine.wrap(looper), prober_2, 0)
+
+test:ok(true, 'no stack overflow with metamethod')
+
+test:done(true)
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 3/3][v3] Add stack check to pcall/xpcall.

[Sergey Bronnikov via Tarantool-patches]

From: Mike Pall <mike>

Analyzed by Peter Cawley.

(cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)

The patch adds the stack check to fast functions `pcall()` and
`xpcall()`.

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

Part of tarantool/tarantool#12134
---
 src/vm_arm.dasc                               |  7 ++++
 src/vm_arm64.dasc                             |  8 +++++
 src/vm_mips.dasc                              | 10 +++++-
 src/vm_mips64.dasc                            | 14 ++++++--
 src/vm_ppc.dasc                               |  9 +++++
 src/vm_x64.dasc                               |  6 ++++
 src/vm_x86.dasc                               |  6 ++++
 ...048-fix-stack-checks-vararg-calls.test.lua | 35 ++++++++++++++++++-
 8 files changed, 90 insertions(+), 5 deletions(-)

diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc
index 7095e660..efe9dcb2 100644
--- a/src/vm_arm.dasc
+++ b/src/vm_arm.dasc
@@ -1201,8 +1201,11 @@ static void build_subroutines(BuildCtx *ctx)
   |//-- Base library: catch errors ----------------------------------------
   |
   |.ffunc pcall
+  |   ldr RB, L->maxstack
+  |   add INS, BASE, NARGS8:RC
   |  ldrb RA, [DISPATCH, #DISPATCH_GL(hookmask)]
   |   cmp NARGS8:RC, #8
+  |   cmphs RB, INS
   |   blo ->fff_fallback
   |  tst RA, #HOOK_ACTIVE		// Remember active hook before pcall.
   |   mov RB, BASE
@@ -1213,7 +1216,11 @@ static void build_subroutines(BuildCtx *ctx)
   |  b ->vm_call_dispatch
   |
   |.ffunc_2 xpcall
+  |   ldr RB, L->maxstack
+  |   add INS, BASE, NARGS8:RC
   |  ldrb RA, [DISPATCH, #DISPATCH_GL(hookmask)]
+  |   cmp RB, INS
+  |   blo ->fff_fallback
   |  checkfunc CARG4, ->fff_fallback	// Traceback must be a function.
   |   mov RB, BASE
   |  strd CARG12, [BASE, #8]		// Swap function and traceback.
diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
index 5ef37243..074c1f31 100644
--- a/src/vm_arm64.dasc
+++ b/src/vm_arm64.dasc
@@ -1166,6 +1166,10 @@ static void build_subroutines(BuildCtx *ctx)
   |//-- Base library: catch errors ----------------------------------------
   |
   |.ffunc pcall
+  |  ldr TMP1, L->maxstack
+  |  add TMP2, BASE, NARGS8:RC
+  |  cmp TMP1, TMP2
+  |  blo ->fff_fallback
   |   cmp NARGS8:RC, #8
   |  ldrb TMP0w, GL->hookmask
   |   blo ->fff_fallback
@@ -1185,6 +1189,10 @@ static void build_subroutines(BuildCtx *ctx)
   |  b ->vm_call_dispatch
   |
   |.ffunc xpcall
+  |  ldr TMP1, L->maxstack
+  |  add TMP2, BASE, NARGS8:RC
+  |  cmp TMP1, TMP2
+  |  blo ->fff_fallback
   |     ldp CARG1, CARG2, [BASE]
   |  ldrb TMP0w, GL->hookmask
   |   subs NARGS8:TMP1, NARGS8:RC, #16
diff --git a/src/vm_mips.dasc b/src/vm_mips.dasc
index 32caabf7..69d09d52 100644
--- a/src/vm_mips.dasc
+++ b/src/vm_mips.dasc
@@ -1382,9 +1382,13 @@ static void build_subroutines(BuildCtx *ctx)
   |//-- Base library: catch errors ----------------------------------------
   |
   |.ffunc pcall
+  |   lw TMP1, L->maxstack
+  |   addu TMP2, BASE, NARGS8:RC
   |  lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
   |  beqz NARGS8:RC, ->fff_fallback
-  |   move TMP2, BASE
+  |.  sltu AT, TMP1, TMP2
+  |   bnez AT, ->fff_fallback
+  |.   move TMP2, BASE
   |   addiu BASE, BASE, 8
   |  // Remember active hook before pcall.
   |  srl TMP3, TMP3, HOOK_ACTIVE_SHIFT
@@ -1394,8 +1398,12 @@ static void build_subroutines(BuildCtx *ctx)
   |.  addiu NARGS8:RC, NARGS8:RC, -8
   |
   |.ffunc xpcall
+  |   lw TMP1, L->maxstack
+  |   addu TMP2, BASE, NARGS8:RC
   |    sltiu AT, NARGS8:RC, 16
   |  lw CARG4, 8+HI(BASE)
+  |   sltu TMP1, TMP1, TMP2
+  |    or AT, AT, TMP1
   |    bnez AT, ->fff_fallback
   |.  lw CARG3, 8+LO(BASE)
   |   lw CARG1, LO(BASE)
diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
index 6c2975b4..4e60ee07 100644
--- a/src/vm_mips64.dasc
+++ b/src/vm_mips64.dasc
@@ -1418,8 +1418,12 @@ static void build_subroutines(BuildCtx *ctx)
   |//-- Base library: catch errors ----------------------------------------
   |
   |.ffunc pcall
+  |  ld TMP1, L->maxstack
+  |  daddu TMP2, BASE, NARGS8:RC
+  |  sltu AT, TMP1, TMP2
+  |  bnez AT, ->fff_fallback
+  |.  lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
   |  daddiu NARGS8:RC, NARGS8:RC, -8
-  |  lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
   |  bltz NARGS8:RC, ->fff_fallback
   |.   move TMP2, BASE
   |   daddiu BASE, BASE, 16
@@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)
   |.  nop
   |
   |.ffunc xpcall
-  |  daddiu NARGS8:TMP0, NARGS8:RC, -16
-  |  ld CARG1, 0(BASE)
+  |  ld TMP1, L->maxstack
+  |  daddu TMP2, BASE, NARGS8:RC
+  |  sltu AT, TMP1, TMP2
+  |  bnez AT, ->fff_fallback
+  |.  ld CARG1, 0(BASE)
+  |  daddiu NARGS8:RC, NARGS8:RC, -16
   |   ld CARG2, 8(BASE)
   |    bltz NARGS8:TMP0, ->fff_fallback
   |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)
diff --git a/src/vm_ppc.dasc b/src/vm_ppc.dasc
index 980ad897..f2ea933b 100644
--- a/src/vm_ppc.dasc
+++ b/src/vm_ppc.dasc
@@ -1755,8 +1755,12 @@ static void build_subroutines(BuildCtx *ctx)
   |//-- Base library: catch errors ----------------------------------------
   |
   |.ffunc pcall
+  |    lwz TMP1, L->maxstack
+  |    add TMP2, BASE, NARGS8:RC
   |  cmplwi NARGS8:RC, 8
   |   lbz TMP3, DISPATCH_GL(hookmask)(DISPATCH)
+  |    cmplw cr1, TMP1, TMP2
+  |  cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt
   |  blt ->fff_fallback
   |   mr TMP2, BASE
   |   la BASE, 8(BASE)
@@ -1767,14 +1771,19 @@ static void build_subroutines(BuildCtx *ctx)
   |  b ->vm_call_dispatch
   |
   |.ffunc xpcall
+  |     lwz TMP1, L->maxstack
+  |     add TMP2, BASE, NARGS8:RC
   |  cmplwi NARGS8:RC, 16
   |   lwz CARG3, 8(BASE)
+  |     cmplw cr1, TMP1, TMP2
   |.if FPU
   |    lfd FARG2, 8(BASE)
+  |  cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt
   |    lfd FARG1, 0(BASE)
   |.else
   |    lwz CARG1, 0(BASE)
   |    lwz CARG2, 4(BASE)
+  |  cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt
   |    lwz CARG4, 12(BASE)
   |.endif
   |  blt ->fff_fallback
diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
index 8b6781a6..c57b76b7 100644
--- a/src/vm_x64.dasc
+++ b/src/vm_x64.dasc
@@ -1545,6 +1545,9 @@ static void build_subroutines(BuildCtx *ctx)
   |//-- Base library: catch errors ----------------------------------------
   |
   |.ffunc_1 pcall
+  |  mov L:RB, SAVE_L
+  |  lea RA, [BASE+NARGS:RD*8]
+  |  cmp RA, L:RB->maxstack; ja ->fff_fallback
   |  lea RA, [BASE+16]
   |  sub NARGS:RDd, 1
   |  mov PCd, 16+FRAME_PCALL
@@ -1563,6 +1566,9 @@ static void build_subroutines(BuildCtx *ctx)
   |  jmp ->vm_call_dispatch
   |
   |.ffunc_2 xpcall
+  |  mov L:RB, SAVE_L
+  |  lea RA, [BASE+NARGS:RD*8]
+  |  cmp RA, L:RB->maxstack; ja ->fff_fallback
   |  mov LFUNC:RA, [BASE+8]
   |  checktp_nc LFUNC:RA, LJ_TFUNC, ->fff_fallback
   |  mov LFUNC:RB, [BASE]		// Swap function and traceback.
diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
index 7c11c78e..36804d11 100644
--- a/src/vm_x86.dasc
+++ b/src/vm_x86.dasc
@@ -1914,6 +1914,9 @@ static void build_subroutines(BuildCtx *ctx)
   |//-- Base library: catch errors ----------------------------------------
   |
   |.ffunc_1 pcall
+  |  mov L:RB, SAVE_L
+  |  lea RA, [BASE+NARGS:RD*8]
+  |  cmp RA, L:RB->maxstack; ja ->fff_fallback
   |  lea RA, [BASE+8]
   |  sub NARGS:RD, 1
   |  mov PC, 8+FRAME_PCALL
@@ -1925,6 +1928,9 @@ static void build_subroutines(BuildCtx *ctx)
   |  jmp ->vm_call_dispatch
   |
   |.ffunc_2 xpcall
+  |  mov L:RB, SAVE_L
+  |  lea RA, [BASE+NARGS:RD*8]
+  |  cmp RA, L:RB->maxstack; ja ->fff_fallback
   |  cmp dword [BASE+12], LJ_TFUNC;  jne ->fff_fallback
   |  mov RB, [BASE+4]			// Swap function and traceback.
   |  mov [BASE+12], RB
diff --git a/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
index 3a8ad63d..ad8b151b 100644
--- a/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
+++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
@@ -5,7 +5,7 @@ local tap = require('tap')
 -- See also https://github.com/LuaJIT/LuaJIT/issues/1048.
 local test = tap.test('lj-1048-fix-stack-checks-vararg-calls')
 
-test:plan(2)
+test:plan(5)
 
 -- The test case demonstrates a segmentation fault due to stack
 -- overflow by recursive calling `pcall()`. The functions are
@@ -50,4 +50,37 @@ pcall(coroutine.wrap(looper), prober_2, 0)
 
 test:ok(true, 'no stack overflow with metamethod')
 
+-- The testcases demonstrates a stack overflow in
+-- `pcall()`/xpcall()` triggered using metamethod `__call`.
+
+t = coroutine.wrap(setmetatable)({}, { __call = pcall })
+
+test:ok(true, 'no stack overflow with metamethod __call with pcall()')
+
+t = coroutine.wrap(setmetatable)({}, { __call = xpcall })
+
+test:ok(true, 'no stack overflow with metamethod __call with xpcall()')
+
+-- The testcase demonstrates a stack overflow in
+-- `pcall()`/`xpcall()` similar to the first testcase, but it is
+-- triggered using `unpack()`.
+
+t = {}
+local function f()
+  return pcall(unpack(t))
+end
+
+-- The problem is only reproduced on LuaJIT GC64 and is best
+-- reproduced under Valgrind than AddressSanitizer. The chosen
+-- value was found experimentally and always results in an attempt
+-- to write beyond the allocated memory.
+local N_ITERATIONS = 200
+
+for i = 1, N_ITERATIONS do
+  t[i], t[i + 1], t[i + 2] = pcall, type, {}
+  coroutine.wrap(f)()
+end
+
+test:ok(true, 'no stack overflow with unpacked pcalls')
+
 test:done(true)
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 0/3][v3] Fix stack overflow in pcall/xpcall

[Sergey Bronnikov via Tarantool-patches]

The proposed patches fixes stack overflow in pcall/xpcall.

Related issues:
- https://github.com/LuaJIT/LuaJIT/issues/1048
- https://github.com/LuaJIT/LuaJIT/issues/1402
- https://github.com/tarantool/tarantool/issues/12134

Git branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-fix-stack-checks-in-vararg-calls

Changes in v2:
- Added patch for MIPS64
- Fixed issues reported on review

Changes in v3:
- Added a new file with test gh-1402-call_init-regression.test.lua
- Adjusted N_ITERATIONS in the last patch with appropriate comment
- Fixups after commentaries from Sergey Kaplun

Mike Pall (3):
  MIPS64: Fix xpcall() error case.
  LJ_FR2: Fix stack checks in vararg calls.
  Add stack check to pcall/xpcall.

 src/lj_def.h                                  |  2 +-
 src/lj_dispatch.c                             |  2 +-
 src/luajit-gdb.py                             |  2 +-
 src/vm_arm.dasc                               |  7 ++
 src/vm_arm64.dasc                             |  9 ++
 src/vm_mips.dasc                              | 10 ++-
 src/vm_mips64.dasc                            | 16 +++-
 src/vm_ppc.dasc                               |  9 ++
 src/vm_x64.dasc                               |  6 ++
 src/vm_x86.dasc                               |  6 ++
 .../gh-1402-call_init-regression.test.lua     | 36 ++++++++
 ...048-fix-stack-checks-vararg-calls.test.lua | 86 +++++++++++++++++++
 12 files changed, 184 insertions(+), 7 deletions(-)
 create mode 100644 test/tarantool-tests/gh-1402-call_init-regression.test.lua
 create mode 100644 test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua

--
2.43.0

Re: [Tarantool-patches] [PATCH luajit 2/3][v3] LJ_FR2: Fix stack checks in vararg calls.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!

LGTM, after fixing my nits below.
Please add the iterational diff for the fixes.

On 12.03.26, Sergey Bronnikov wrote:
> From: Mike Pall <mike>
> 
> Thanks to Peter Cawley.
> 
> (cherry picked from commit d1a2fef8a8f53b0055ee041f7f63d83a27444ffa)
> 
> Stack overflow can cause a segmentation fault in a vararg
> function on ARM64 and MIPS64 in LJ_FR2 mode. This happens
> because the stack check in BC_IFUNCV is off by one on these
> platforms without the patch. The original stack check
> for ARM64 and MIPS64 was incorrect:
> 
> | RA == BASE + (RD=NARGS)*8 + framesize * 8 >= maxstack
> 
> while the stack check on x86_64 is correct and therefore is
> not affected by the problem:
> 
> | RA == BASE + (RD=NARGS+1)*8 + framesize * 8 +8 > maxstack

Typo: s/ +8/ + 8/

> 
> The patch partially fixes the aforementioned issue by bumping
> LJ_STACK_EXTRA by 1 to give a space to the entire frame link for a
> vararg function as the __newindex metamethod.
> 
> A fixup for a number of required slots in `call_init()` was added
> for consistency with non-GC64 flavor. The check is too strict, so
> this can't lead to any crash.
> 
> This patch also corrects the number of redzone slots in
> luajit-gdb.py to match the updated LJ_STACK_EXTRA and adds the test

luajit_lldb.py should be updated as well.

> <gh-1402-call_init-regression.test.lua> that will help to avoid

gh- prefix is for the Tarantool issue tracker, use lj- for LuaJIT issue
tracker.

> a regression in the future, see details in [1].

Just mention details here like the following:

| The patch partially fixes the aforementioned issue by bumping
| LJ_STACK_EXTRA by 1 to give a space to the entire frame link for a
| vararg function as the __newindex metamethod.
|
| A fixup for a number of required slots in `call_init()` was added for
| consistency with the non-GC64 flavor. The check is too strict (if
| comparing the corresponding checks in the VM BC_IFUNCV), so this can't
| lead to any crash. To avoid possible regression in the future the
| corresponding test is added.
|
| This patch also corrects the number of redzone slots in luajit-gdb.py
| and luajit_lldb.py to match the updated LJ_STACK_EXTRA.

> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#12134
> 
> 1. https://github.com/LuaJIT/LuaJIT/issues/1402

Please, don't mention the issue during backporting, to avoid messing the
issue tracker.

> ---
>  src/lj_def.h                                  |  2 +-
>  src/lj_dispatch.c                             |  2 +-
>  src/luajit-gdb.py                             |  2 +-
>  src/vm_arm64.dasc                             |  1 +
>  src/vm_mips64.dasc                            |  1 +
>  .../gh-1402-call_init-regression.test.lua     | 36 +++++++++++++

gh- prefix is for the Tarantool issue tracker, use lj- for LuaJIT issue
tracker.

>  ...048-fix-stack-checks-vararg-calls.test.lua | 53 +++++++++++++++++++
>  7 files changed, 94 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-1402-call_init-regression.test.lua
>  create mode 100644 test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
> 
> diff --git a/src/lj_def.h b/src/lj_def.h
> index a5bca6b0..7e4f251e 100644
> --- a/src/lj_def.h
> +++ b/src/lj_def.h

<snipped>

> diff --git a/src/lj_dispatch.c b/src/lj_dispatch.c
> index a44a5adf..431cb3c2 100644
> --- a/src/lj_dispatch.c
> +++ b/src/lj_dispatch.c

<snipped>

> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> index 0ae2a6e0..dab07b35 100644
> --- a/src/luajit-gdb.py
> +++ b/src/luajit-gdb.py

<snipped>

> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
> index 6600e226..5ef37243 100644
> --- a/src/vm_arm64.dasc
> +++ b/src/vm_arm64.dasc

<snipped>

> diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
> index da187a7a..6c2975b4 100644
> --- a/src/vm_mips64.dasc
> +++ b/src/vm_mips64.dasc

<snipped>

> diff --git a/test/tarantool-tests/gh-1402-call_init-regression.test.lua b/test/tarantool-tests/gh-1402-call_init-regression.test.lua

Please, avoid _ in the file names, lets name it like:

lj-1402-vararg-stkov-check-gc64.test.lua

Same for the name of the test.

> new file mode 100644
> index 00000000..b20f9e39
> --- /dev/null
> +++ b/test/tarantool-tests/gh-1402-call_init-regression.test.lua
> @@ -0,0 +1,36 @@
> +local tap = require('tap')
> +
> +-- A test file to demonstrate a probably quite strict stack
> +-- check for vararg functions in call_init.

This is not about quite strict stack check. We need this to test the
behaviour of the LuaJIT while recording the vararg function. Let's
rephrase like the following:

| -- The test file to verify correctness of stack size check during
| -- recording of vararg functions.

The test file to verify correctness of stack size check during recording of vararg functions.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/1402
> +local test = tap.test('gh-1402-call_init-regression.test.lua'):skipcond({

gh- prefix is for the Tarantool issue tracker, use lj- for LuaJIT issue
tracker.

> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local function vararg(...) -- luacheck: no unused

Let's use this comment before the vararg declaration.
It helps with the _ below as well.

> +  -- None.
> +end
> +
> +-- Make compilation aggressive.

Excess comment. It's quite general approach in our tests.

> +jit.opt.start("hotloop=1")

Typo: s/"/'/g

> +

Please add the following comment:

| -- This function utilizes the exact amount of stack slots
| -- to cause the stack reallocation during `call_init()` in the
| -- GC64 mode.

> +local function caller()
> +  -- luacheck: push no unused

Lets drop this luacheck suppression, see the comment above.

> +  local _, _, _, _, _, _, _, _, _, _
> +  local _, _, _, _, _, _, _, _, _, _
> +  local _, _, _, _, _, _, _, _, _, _
> +  -- luacheck: pop
> +  local n = 1
> +  while n < 3 do
> +    vararg()
> +    n = n + 1
> +  end
> +end
> +
> +pcall(coroutine.wrap(caller))

The pcall is excess lets do it without it:
| coroutine.wrap(caller)()

> +
> +test:ok(true, 'no assertion for vararg functions in call_init')

Just mention 'no assertion failure' (this assertion isn't in the
`call_init()`, but during recording in `rec_check_slots()`).

> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
> new file mode 100644
> index 00000000..3a8ad63d
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 3/3][v3] Add stack check to pcall/xpcall.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
Please, fix my comments below.

Don't forget to add the corresponding iterative changes.

On 12.03.26, Sergey Bronnikov wrote:
> From: Mike Pall <mike>
> 
> Analyzed by Peter Cawley.
> 
> (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
> 
> The patch adds the stack check to fast functions `pcall()` and
> `xpcall()`.

Please add more verbose description:

| (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
|
| The `pcall()` and `xpcall()` calls in GC64 mode require 2 slots. This
| means that all arguments should be moved up during emitting of the frame
| link to the stack. Hence, this may cause stack overflow without the
| corresponding check.
|
| This patch adds the corresponding checks to the VM. Non-GC64 VMs are
| updated as well for the consistency.

> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#12134
> ---
>  src/vm_arm.dasc                               |  7 ++++
>  src/vm_arm64.dasc                             |  8 +++++
>  src/vm_mips.dasc                              | 10 +++++-
>  src/vm_mips64.dasc                            | 14 ++++++--
>  src/vm_ppc.dasc                               |  9 +++++
>  src/vm_x64.dasc                               |  6 ++++
>  src/vm_x86.dasc                               |  6 ++++
>  ...048-fix-stack-checks-vararg-calls.test.lua | 35 ++++++++++++++++++-
>  8 files changed, 90 insertions(+), 5 deletions(-)
> 
> diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc
> index 7095e660..efe9dcb2 100644
> --- a/src/vm_arm.dasc
> +++ b/src/vm_arm.dasc

<snipped>

> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
> index 5ef37243..074c1f31 100644
> --- a/src/vm_arm64.dasc
> +++ b/src/vm_arm64.dasc

<snipped>

> diff --git a/src/vm_mips.dasc b/src/vm_mips.dasc
> index 32caabf7..69d09d52 100644
> --- a/src/vm_mips.dasc
> +++ b/src/vm_mips.dasc

<snipped>

> diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
> index 6c2975b4..4e60ee07 100644
> --- a/src/vm_mips64.dasc
> +++ b/src/vm_mips64.dasc
> @@ -1418,8 +1418,12 @@ static void build_subroutines(BuildCtx *ctx)
>    |//-- Base library: catch errors ----------------------------------------
>    |
>    |.ffunc pcall
> +  |  ld TMP1, L->maxstack
> +  |  daddu TMP2, BASE, NARGS8:RC
> +  |  sltu AT, TMP1, TMP2
> +  |  bnez AT, ->fff_fallback
> +  |.  lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
>    |  daddiu NARGS8:RC, NARGS8:RC, -8
> -  |  lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
>    |  bltz NARGS8:RC, ->fff_fallback
>    |.   move TMP2, BASE
>    |   daddiu BASE, BASE, 16
> @@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)
>    |.  nop
>    |
>    |.ffunc xpcall
> -  |  daddiu NARGS8:TMP0, NARGS8:RC, -16

This neglets the first patch in the series. See the comment below.

> -  |  ld CARG1, 0(BASE)
> +  |  ld TMP1, L->maxstack
> +  |  daddu TMP2, BASE, NARGS8:RC
> +  |  sltu AT, TMP1, TMP2
> +  |  bnez AT, ->fff_fallback
> +  |.  ld CARG1, 0(BASE)

> +  |  daddiu NARGS8:RC, NARGS8:RC, -16

This line is incorrect. This neglets the 1st patch in the series.

It should be
| |  daddiu NARGS8:TMP0, NARGS8:RC, -16


>    |   ld CARG2, 8(BASE)
>    |    bltz NARGS8:TMP0, ->fff_fallback
>    |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)
> diff --git a/src/vm_ppc.dasc b/src/vm_ppc.dasc
> index 980ad897..f2ea933b 100644
> --- a/src/vm_ppc.dasc
> +++ b/src/vm_ppc.dasc

<snipped>

> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
> index 8b6781a6..c57b76b7 100644
> --- a/src/vm_x64.dasc
> +++ b/src/vm_x64.dasc

<snipped>

> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
> index 7c11c78e..36804d11 100644
> --- a/src/vm_x86.dasc
> +++ b/src/vm_x86.dasc

<snipped>

> diff --git a/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
> index 3a8ad63d..ad8b151b 100644
> --- a/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
> +++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
> @@ -5,7 +5,7 @@ local tap = require('tap')
>  -- See also https://github.com/LuaJIT/LuaJIT/issues/1048.
>  local test = tap.test('lj-1048-fix-stack-checks-vararg-calls')
>  
> -test:plan(2)
> +test:plan(5)
>  
>  -- The test case demonstrates a segmentation fault due to stack
>  -- overflow by recursive calling `pcall()`. The functions are
> @@ -50,4 +50,37 @@ pcall(coroutine.wrap(looper), prober_2, 0)
>  
>  test:ok(true, 'no stack overflow with metamethod')
>  
> +-- The testcases demonstrates a stack overflow in
> +-- `pcall()`/xpcall()` triggered using metamethod `__call`.
> +
> +t = coroutine.wrap(setmetatable)({}, { __call = pcall })

I've meant the following:

| t = setmetatable({}, { __call = pcall })
| coroutine.wrap(function() t() end)()

> +
> +test:ok(true, 'no stack overflow with metamethod __call with pcall()')
> +
> +t = coroutine.wrap(setmetatable)({}, { __call = xpcall })

I've meant the following:

| t = setmetatable({}, { __call = xpcall })
| coroutine.wrap(function() t() end)()

But this won't work since the second amount of xpcall must be the
function. So, this test case is invalid. We must to duplicate the second
approach with `xpcall()`

This works fine.
| LUA_PATH="src/?.lua;;" gdb --args src/luajit -e '
| local t = {}
| local function xpcall_wrapper()
|   return xpcall(unpack(t))
| end
|
| local N_ITERATIONS = 200
|
| for i = 1, N_ITERATIONS do
|   t[i], t[i + 1], t[i + 2] = xpcall, type, {}
|   coroutine.wrap(xpcall_wrapper)()
| end
| '

> +
> +test:ok(true, 'no stack overflow with metamethod __call with xpcall()')
> +
> +-- The testcase demonstrates a stack overflow in
> +-- `pcall()`/`xpcall()` similar to the first testcase, but it is
> +-- triggered using `unpack()`.
> +
> +t = {}
> +local function f()

Lets name it `pcall_wrapper()`

> +  return pcall(unpack(t))
> +end
> +
> +-- The problem is only reproduced on LuaJIT GC64 and is best

Typo: s/best/better/

> +-- reproduced under Valgrind than AddressSanitizer. The chosen
> +-- value was found experimentally and always results in an attempt
> +-- to write beyond the allocated memory.
> +local N_ITERATIONS = 200
> +
> +for i = 1, N_ITERATIONS do
> +  t[i], t[i + 1], t[i + 2] = pcall, type, {}
> +  coroutine.wrap(f)()
> +end
> +
> +test:ok(true, 'no stack overflow with unpacked pcalls')
> +
>  test:done(true)
> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun