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

[Tarantool-patches] [PATCH luajit 0/2] 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/tarantool/tarantool/issues/11691
Git branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-fix-stack-checks-in-vararg-calls

Mike Pall (2):
  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/vm_arm.dasc                               |  7 ++
 src/vm_arm64.dasc                             |  9 ++
 src/vm_mips.dasc                              | 10 ++-
 src/vm_mips64.dasc                            | 13 ++-
 src/vm_ppc.dasc                               |  9 ++
 src/vm_x64.dasc                               |  6 ++
 src/vm_x86.dasc                               |  6 ++
 ...048-fix-stack-checks-vararg-calls.test.lua | 85 +++++++++++++++++++
 10 files changed, 144 insertions(+), 5 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua

-- 
2.43.0

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

[Sergey Bronnikov via Tarantool-patches]

Thanks to Peter Cawley.

(cherry picked from commit d1a2fef8a8f53b0055ee041f7f63d83a27444ffa)

The builtin `pcall()` has two separate ways by which it can
grow the stack by one slot:

1. Resolving the `__call` metamethod of its first argument.
2. Growing the stack by one slot in LJ_FR2 mode.

The first case leads to a stack smash if `pcall()` is used as
`__call`. Setting a metatable with this metamethod will cause
an infinite loop which fills up the stack with `pcall`-frames
and then keeps going beyond the end of the stack until it segfaults.
Either of these points can cause an issue if `pcall()` is used as
`__newindex`. The patch partially fixes aforementioned issues.

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

Part of tarantool/tarantool#11691
---
 src/lj_def.h                                  |  2 +-
 src/lj_dispatch.c                             |  2 +-
 src/vm_arm64.dasc                             |  1 +
 src/vm_mips64.dasc                            |  1 +
 ...048-fix-stack-checks-vararg-calls.test.lua | 56 +++++++++++++++++++
 5 files changed, 60 insertions(+), 2 deletions(-)
 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/vm_arm64.dasc b/src/vm_arm64.dasc
index c5f0a7a7..cf8e575a 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 44fba36c..7f49df5b 100644
--- a/src/vm_mips64.dasc
+++ b/src/vm_mips64.dasc
@@ -5267,6 +5267,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/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..e300d5c1
--- /dev/null
+++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
@@ -0,0 +1,56 @@
+local tap = require('tap')
+
+-- A test file to demonstrate a stack overflow in `pcall()` in
+-- some cases, see below testcase descriptions.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1048.
+local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(2)
+
+-- The first testcase demonstrate a stack overflow in `pcall()`
+-- by recursive calling `pcall()`. The functions are vararg
+-- because stack check in BC_IFUNCV is off by one without the
+-- patch.
+local function prober_1(...) -- luacheck: no unused
+  pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, pairs, {})
+end
+
+local function looper_1(n, ...)
+  prober_1(...)
+  prober_1(nil, ...)
+  return looper_1(n + 1, n, ...)
+end
+
+pcall(coroutine.wrap(looper_1), 0)
+
+test:ok(true, 'no stack overflow with recursive pcall')
+
+-- The second testcase demonstrate a stack overflow in `pcall()`
+-- with using metamethods. A stack overflow is triggered when
+-- `pcall()` is used as `__call` metamethod, setting metatable
+-- will cause an infinite loop which fills up the stack with
+-- `pcall`-frames and then keeps going beyond the end of the
+-- stack until it segfaults. Also, a stack overflow can be
+-- triggered when `pcall()` is used as `__newindex` metamethod.
+-- The functions are vararg because stack check in BC_IFUNCV is
+-- off by one without the patch.
+
+local mt = setmetatable({}, { __newindex = pcall, __call = pairs })
+
+local function prober_2(...) -- luacheck: no unused
+  mt[mt] = mt
+end
+
+local function looper_2(n, ...)
+  prober_2(...)
+  prober_2(nil, ...)
+  return looper_2(n + 1, n, ...)
+end
+
+pcall(coroutine.wrap(looper_2), 0)
+
+test:ok(true, 'no stack overflow with using metamethod')
+
+test:done(true)
-- 
2.43.0

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

[Sergey Bronnikov via Tarantool-patches]

Analyzed by Peter Cawley.

(cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)

In the commit "LJ_FR2: Fix stack checks in vararg calls."
stack overflow in `pcall()`/`xpcall()` was fixed partially and
there are still cases where stack overflow happens, see comments
in the test. The patch add stack check to `pcall()` and `xpcall()`.

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

Part of tarantool/tarantool#11691
---
 src/vm_arm.dasc                               |  7 +++++
 src/vm_arm64.dasc                             |  8 +++++
 src/vm_mips.dasc                              | 10 +++++-
 src/vm_mips64.dasc                            | 12 +++++--
 src/vm_ppc.dasc                               |  9 ++++++
 src/vm_x64.dasc                               |  6 ++++
 src/vm_x86.dasc                               |  6 ++++
 ...048-fix-stack-checks-vararg-calls.test.lua | 31 ++++++++++++++++++-
 8 files changed, 85 insertions(+), 4 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 cf8e575a..53ff7162 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 7f49df5b..06b143a2 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
+  |  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 CARG1, 0(BASE)
   |   ld CARG2, 8(BASE)
   |    bltz NARGS8:RC, ->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 d5296759..141f5f82 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 b043b830..1ba5abce 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 e300d5c1..367aecb6 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
@@ -7,7 +7,7 @@ local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
 
-test:plan(2)
+test:plan(4)
 
 -- The first testcase demonstrate a stack overflow in `pcall()`
 -- by recursive calling `pcall()`. The functions are vararg
@@ -53,4 +53,33 @@ pcall(coroutine.wrap(looper_2), 0)
 
 test:ok(true, 'no stack overflow with using metamethod')
 
+-- The third testcase demonstrate a stack overflow in
+-- `pcall()`/xpcall()` similar to the first testcase, but it is
+-- triggered using hand-crafted Lua chunk with a lot `pcall()`
+-- builtins.
+
+for i = 1, 100 do
+  local code = 'return pcall(' .. string.rep('pcall, ', i) .. 'pairs, {})'
+  local f = load(code)
+  coroutine.wrap(f)()
+end
+
+test:ok(true, 'no stack overflow with pcalls in load()')
+
+-- The fourth testcase demonstrate a stack overflow in
+-- `pcall()`/`xpcall()` similar to the first testcase, but it is
+-- triggered using `unpack()`.
+
+local t = {}
+local function f()
+  return pcall(unpack(t))
+end
+
+for i = 1, 100 do
+  t[i], t[i + 1], t[i + 2] = pcall, pairs, {}
+  coroutine.wrap(f)()
+end
+
+test:ok(true, 'no stack overflow with unpacked pcalls')
+
 test:done(true)
-- 
2.43.0

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

[Sergey Kaplun via Tarantool-patches]

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

On 27.08.25, Sergey Bronnikov wrote:
> Thanks to Peter Cawley.
> 
> (cherry picked from commit d1a2fef8a8f53b0055ee041f7f63d83a27444ffa)
> 
> The builtin `pcall()` has two separate ways by which it can
> grow the stack by one slot:
> 
> 1. Resolving the `__call` metamethod of its first argument.

This is unrelated to this patch, so it can be omitted.

> 2. Growing the stack by one slot in LJ_FR2 mode.
> 
> The first case leads to a stack smash if `pcall()` is used as
> `__call`. Setting a metatable with this metamethod will cause
> an infinite loop which fills up the stack with `pcall`-frames
> and then keeps going beyond the end of the stack until it segfaults.

This issue is not related to this patch.

> Either of these points can cause an issue if `pcall()` is used as
> `__newindex`.

Looks like the metamethods are not required for issue reproducing.

>               The patch partially fixes aforementioned issues.

By how?

> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#11691
> ---
>  src/lj_def.h                                  |  2 +-
>  src/lj_dispatch.c                             |  2 +-
>  src/vm_arm64.dasc                             |  1 +
>  src/vm_mips64.dasc                            |  1 +
>  ...048-fix-stack-checks-vararg-calls.test.lua | 56 +++++++++++++++++++
>  5 files changed, 60 insertions(+), 2 deletions(-)
>  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

<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
> @@ -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;

I can't see the test related to this change. Not `prober_1()` nor
`prober_2()` lead to the assertion failure for x86_64 or aarch64 without
it.

>      lj_state_checkstack(L, (MSize)need);
>      numparams -= gotparams;
>      return numparams >= 0 ? numparams : 0;
> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
> index c5f0a7a7..cf8e575a 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

Please mention in the commit message why the original stack check was
incorrect (for aarch64 and mips64).

Also, mention why the x64 isn't affected:

x64:
| RA == BASE + (RD=NARGS+1)*8 + framesize * 8 +8 > maxstack
The last summand here is the `LJ_FR2` adjustment.

arm64|mips64 -- incorrect check:
| RA == BASE + (RD=NARGS)*8 + framesize * 8 >= maxstack


>      |   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 44fba36c..7f49df5b 100644
> --- a/src/vm_mips64.dasc
> +++ b/src/vm_mips64.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
> new file mode 100644
> index 00000000..e300d5c1
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
> @@ -0,0 +1,56 @@
> +local tap = require('tap')
> +
> +-- A test file to demonstrate a stack overflow in `pcall()` in
> +-- some cases, see below testcase descriptions.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/1048.
> +local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(2)
> +
> +-- The first testcase demonstrate a stack overflow in `pcall()`
> +-- by recursive calling `pcall()`. The functions are vararg
> +-- because stack check in BC_IFUNCV is off by one without the

Minor: by one for the arm64, mips64 architectures.

> +-- patch.
> +local function prober_1(...) -- luacheck: no unused
> +  pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, pairs, {})
> +end

Why do we want to use probber_1 here? Why is this different from the
second example? Only because of the metamethods?

If we want to keep it, please describe why we need at least 9 pcall-s.

Also, there is no need for `pairs()` here. Let's use another simpler fast
function (like `type()`). Also, please add a comment about fast function
usage, see the example below.

> +
> +local function looper_1(n, ...)
> +  prober_1(...)
> +  prober_1(nil, ...)

Why do we need `nil` here? I suppose this line is excess, see the
comment with the example below.

> +  return looper_1(n + 1, n, ...)
> +end
> +
> +pcall(coroutine.wrap(looper_1), 0)
> +
> +test:ok(true, 'no stack overflow with recursive pcall')
> +
> +-- The second testcase demonstrate a stack overflow in `pcall()`
> +-- with using metamethods. A stack overflow is triggered when
> +-- `pcall()` is used as `__call` metamethod, setting metatable
> +-- will cause an infinite loop which fills up the stack with
> +-- `pcall`-frames and then keeps going beyond the end of the
> +-- stack until it segfaults.

This comment is unrelated to this test.

>                                Also, a stack overflow can be
> +-- triggered when `pcall()` is used as `__newindex` metamethod.
> +-- The functions are vararg because stack check in BC_IFUNCV is
> +-- off by one without the patch.
> +
> +local mt = setmetatable({}, { __newindex = pcall, __call = pairs })
> +
> +local function prober_2(...) -- luacheck: no unused
> +  mt[mt] = mt
> +end
> +
> +local function looper_2(n, ...)
> +  prober_2(...)
> +  prober_2(nil, ...)
> +  return looper_2(n + 1, n, ...)
> +end
> +
> +pcall(coroutine.wrap(looper_2), 0)

This can be simplified to the following:
| src/luajit -e '
| -- Do not use a Lua function as metamethod -- since it will check
| -- the stack on each invocation. Use simple `type()` built-in
| -- instead.
| local t = setmetatable({}, {__newindex = pcall, __call = type})
| local function prober(...)
|     -- Invokes `pcall(t, t, t)`.
|     t[t] = t
| end
| local function looper(n, ...)
|     prober(...)
|     return looper(n+1, n, ...)
| end
| pcall(coroutine.wrap(looper), 0)
| '

> +
> +test:ok(true, 'no stack overflow with using metamethod')
> +
> +test:done(true)
> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

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

On 27.08.25, Sergey Bronnikov wrote:
> Analyzed by Peter Cawley.
> 
> (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
> 
> In the commit "LJ_FR2: Fix stack checks in vararg calls."

Minor: In the previous commit ("...")

> stack overflow in `pcall()`/`xpcall()` was fixed partially and

This handles stack overflow for vararg functions and metamethod
invocations not `xpcall()/pcall()` (directly).

> there are still cases where stack overflow happens, see comments
> in the test. The patch add stack check to `pcall()` and `xpcall()`.

Please, mention in the commit message that the issue was fixed by adding
the stack check to these fast functions.

> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#11691
> ---
>  src/vm_arm.dasc                               |  7 +++++
>  src/vm_arm64.dasc                             |  8 +++++
>  src/vm_mips.dasc                              | 10 +++++-
>  src/vm_mips64.dasc                            | 12 +++++--
>  src/vm_ppc.dasc                               |  9 ++++++
>  src/vm_x64.dasc                               |  6 ++++
>  src/vm_x86.dasc                               |  6 ++++
>  ...048-fix-stack-checks-vararg-calls.test.lua | 31 ++++++++++++++++++-
>  8 files changed, 85 insertions(+), 4 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 cf8e575a..53ff7162 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 7f49df5b..06b143a2 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

I see that the original patch in the upstream has another diff. Please
backport the commit ea7071d3 ("MIPS64: Fix xpcall() error case") first
(as the first commit in the patch series) to avoid conflicts in the
future.

> @@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)
>    |.  nop
>    |
>    |.ffunc xpcall
> +  |  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 CARG1, 0(BASE)
>    |   ld CARG2, 8(BASE)
>    |    bltz NARGS8:RC, ->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 d5296759..141f5f82 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 b043b830..1ba5abce 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 e300d5c1..367aecb6 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
> @@ -7,7 +7,7 @@ local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({
>    ['Test requires JIT enabled'] = not jit.status(),
>  })
>  
> -test:plan(2)
> +test:plan(4)
>  
>  -- The first testcase demonstrate a stack overflow in `pcall()`
>  -- by recursive calling `pcall()`. The functions are vararg
> @@ -53,4 +53,33 @@ pcall(coroutine.wrap(looper_2), 0)
>  
>  test:ok(true, 'no stack overflow with using metamethod')
>  

Why do you drop the original test case?

Expected behaviour:
| src/luajit -e 'local t = {setmetatable({},{__call=pcall})()} print(t[#t])'
| stack overflow
Actual behaviour -- dirty read detected by ASAN.

> +-- The third testcase demonstrate a stack overflow in
> +-- `pcall()`/xpcall()` similar to the first testcase, but it is
> +-- triggered using hand-crafted Lua chunk with a lot `pcall()`
> +-- builtins.
> +
> +for i = 1, 100 do
> +  local code = 'return pcall(' .. string.rep('pcall, ', i) .. 'pairs, {})'

Why do we need this test if it has the same semantics as the second
one?

> +  local f = load(code)
> +  coroutine.wrap(f)()
> +end
> +
> +test:ok(true, 'no stack overflow with pcalls in load()')
> +
> +-- The fourth testcase demonstrate a stack overflow in
> +-- `pcall()`/`xpcall()` similar to the first testcase, but it is
> +-- triggered using `unpack()`.
> +
> +local t = {}
> +local function f()
> +  return pcall(unpack(t))
> +end
> +
> +for i = 1, 100 do

This limit isn't enough for GC64 or non-GC64 mode.
| src/luajit -e '
| local t = {}
| local function f() return pcall(unpack(t)) end
| for i = 1, 100 do
|   t[i], t[i+1], t[i+2] = pcall, pairs, {}
|   coroutine.wrap(f)()
| end
| '

For the GC64 build it is necessary to set the limit as 180, (179 -- not
SegFault).

Please provide two different limits depending on the GC64 mode
configuration. Please, describe why the __exact__ limit is chosen for
the particular configuration.

> +  t[i], t[i + 1], t[i + 2] = pcall, pairs, {}
> +  coroutine.wrap(f)()
> +end
> +
> +test:ok(true, 'no stack overflow with unpacked pcalls')
> +
>  test:done(true)
> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 14041 bytes --]

Hi, Sergey,

thanks for review! Please see my comments below.

Sergey

On 9/1/25 16:07, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On 27.08.25, Sergey Bronnikov wrote:
>> Thanks to Peter Cawley.
>>
>> (cherry picked from commit d1a2fef8a8f53b0055ee041f7f63d83a27444ffa)
>>
>> The builtin `pcall()` has two separate ways by which it can
>> grow the stack by one slot:
>>
>> 1. Resolving the `__call` metamethod of its first argument.
> This is unrelated to this patch, so it can be omitted.
>
>> 2. Growing the stack by one slot in LJ_FR2 mode.
>>
>> The first case leads to a stack smash if `pcall()` is used as
>> `__call`. Setting a metatable with this metamethod will cause
>> an infinite loop which fills up the stack with `pcall`-frames
>> and then keeps going beyond the end of the stack until it segfaults.
> This issue is not related to this patch.
>
>> Either of these points can cause an issue if `pcall()` is used as
>> `__newindex`.
> Looks like the metamethods are not required for issue reproducing.
>
>>                The patch partially fixes aforementioned issues.
> By how?

I've updated the commit message as the following:

     Stack overflow can cause a segmentation fault in vararg
     function on ARM64 and MIPS64 in LJ_FR2 mode. This happen
     because stack check in BC_IFUNCV is off by one on these
     platforms without the patch. The patch partially fixes
     aforementioned issue by bumping LJ_STACK_EXTRA by 1 to
     give a space to write the entire frame link and fixing
     a number of last free slot in the stack.

>
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#11691
>> ---
>>   src/lj_def.h                                  |  2 +-
>>   src/lj_dispatch.c                             |  2 +-
>>   src/vm_arm64.dasc                             |  1 +
>>   src/vm_mips64.dasc                            |  1 +
>>   ...048-fix-stack-checks-vararg-calls.test.lua | 56 +++++++++++++++++++
>>   5 files changed, 60 insertions(+), 2 deletions(-)
>>   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
> <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
>> @@ -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;
> I can't see the test related to this change. Not `prober_1()` nor
> `prober_2()` lead to the assertion failure for x86_64 or aarch64 without
> it.

Please check again. Both testcases trigger segfault on AArch64 (odroid).

cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug  -DLUA_USE_ASSERT=ON 
-DLUA_USE_APICHECK=ON

cmake --build build --parallel

LUA_PATH="/root/sergeyb/luajit/test/tarantool-tests/?.lua;/root/sergeyb/luajit/test/tarantool-tests/?/init.lua;/root/sergeyb/luajit/src/?.lua;/root/sergeyb/luajit/build/src/?.lua;;" 
gdb --args /root/sergeyb/luajit/build/src/luajit "-e" 
"dofile[[/root/sergeyb/luajit/test/luajit-test-init.lua]]" 
"/root/sergeyb/luajit/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua"

(gdb) bt

#0  0x00000055555c16f4 in lj_alloc_free (msp=0x7fb7d56010, ptr=0x7fb7d69088)
     at /root/sergeyb/luajit/src/lj_alloc.c:1405
#1  0x00000055555c1fe4 in lj_alloc_realloc (msp=0x7fb7d56010, 
ptr=0x7fb7d69088, nsize=1696)
     at /root/sergeyb/luajit/src/lj_alloc.c:1471
#2  0x00000055555c204c in lj_alloc_f (msp=0x7fb7d56010, 
ptr=0x7fb7d69088, osize=816, nsize=1696)
     at /root/sergeyb/luajit/src/lj_alloc.c:1486
#3  0x00000055555790e0 in lj_mem_realloc (L=0x7fb7d6d330, 
p=0x7fb7d69088, osz=816, nsz=1696)
     at /root/sergeyb/luajit/src/lj_gc.c:896
#4  0x000000555557e610 in resizestack (L=0x7fb7d6d330, n=204) at 
/root/sergeyb/luajit/src/lj_state.c:82
#5  0x000000555557e970 in lj_state_growstack (L=0x7fb7d6d330, need=48)
     at /root/sergeyb/luajit/src/lj_state.c:130
#6  0x00000055555fad68 in lj_vm_growstack_l () at buildvm_arm64.dasc:1263
#7  0x00000055555fb8d4 in lj_ff_coroutine_wrap_aux () at 
buildvm_arm64.dasc:1775
#8  0x000000555556a824 in lua_pcall (L=0x7fb7d56378, nargs=0, 
nresults=-1, errfunc=2)
     at /root/sergeyb/luajit/src/lj_api.c:1173
#9  0x000000555555d258 in docall (L=0x7fb7d56378, narg=0, clear=0) at 
/root/sergeyb/luajit/src/luajit.c:134
#10 0x000000555555db9c in handle_script (L=0x7fb7d56378, argx=0x7ffffff280)
     at /root/sergeyb/luajit/src/luajit.c:304
#11 0x000000555555ea54 in pmain (L=0x7fb7d56378) at 
/root/sergeyb/luajit/src/luajit.c:602
#12 0x00000055555fab90 in lj_BC_FUNCC () at buildvm_arm64.dasc:894
#13 0x000000555556ad90 in lua_cpcall (L=0x7fb7d56378, func=0x555555e898 
<pmain>, ud=0x0)
     at /root/sergeyb/luajit/src/lj_api.c:1208
#14 0x000000555555ebb4 in main (argc=4, argv=0x7ffffff268) at 
/root/sergeyb/luajit/src/luajit.c:633

(gdb)

With commented out first testcase:

(gdb) bt
#0  0x00000055555c18fc in lj_alloc_free (msp=0x7fb7d56010, ptr=0x7fb7d69068)
     at /root/sergeyb/luajit/src/lj_alloc.c:1406
#1  0x00000055555c1fe4 in lj_alloc_realloc (msp=0x7fb7d56010, 
ptr=0x7fb7d69068, nsize=1696)
     at /root/sergeyb/luajit/src/lj_alloc.c:1471
#2  0x00000055555c204c in lj_alloc_f (msp=0x7fb7d56010, 
ptr=0x7fb7d69068, osize=816, nsize=1696)
     at /root/sergeyb/luajit/src/lj_alloc.c:1486
#3  0x00000055555790e0 in lj_mem_realloc (L=0x7fb7d6d2a0, 
p=0x7fb7d69068, osz=816, nsz=1696)
     at /root/sergeyb/luajit/src/lj_gc.c:896
#4  0x000000555557e610 in resizestack (L=0x7fb7d6d2a0, n=204) at 
/root/sergeyb/luajit/src/lj_state.c:82
#5  0x000000555557e970 in lj_state_growstack (L=0x7fb7d6d2a0, need=48)
     at /root/sergeyb/luajit/src/lj_state.c:130
#6  0x00000055555fad68 in lj_vm_growstack_l () at buildvm_arm64.dasc:1263
#7  0x00000055555fb8d4 in lj_ff_coroutine_wrap_aux () at 
buildvm_arm64.dasc:1775
#8  0x000000555556a824 in lua_pcall (L=0x7fb7d56378, nargs=0, 
nresults=-1, errfunc=2)
     at /root/sergeyb/luajit/src/lj_api.c:1173
#9  0x000000555555d258 in docall (L=0x7fb7d56378, narg=0, clear=0) at 
/root/sergeyb/luajit/src/luajit.c:134
#10 0x000000555555db9c in handle_script (L=0x7fb7d56378, argx=0x7ffffff280)
     at /root/sergeyb/luajit/src/luajit.c:304
#11 0x000000555555ea54 in pmain (L=0x7fb7d56378) at 
/root/sergeyb/luajit/src/luajit.c:602
#12 0x00000055555fab90 in lj_BC_FUNCC () at buildvm_arm64.dasc:894
#13 0x000000555556ad90 in lua_cpcall (L=0x7fb7d56378, func=0x555555e898 
<pmain>, ud=0x0)
     at /root/sergeyb/luajit/src/lj_api.c:1208
#14 0x000000555555ebb4 in main (argc=4, argv=0x7ffffff268) at 
/root/sergeyb/luajit/src/luajit.c:633
(gdb)


>>       lj_state_checkstack(L, (MSize)need);
>>       numparams -= gotparams;
>>       return numparams >= 0 ? numparams : 0;
>> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
>> index c5f0a7a7..cf8e575a 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
>>       |   addLFUNC:CARG3, CARG3, TMP0, lsl #47
>>       |  add RA, RA, RC
>> +    |  sub CARG1, CARG1, #8
> Please mention in the commit message why the original stack check was
> incorrect (for aarch64 and mips64).
>
> Also, mention why the x64 isn't affected:
>
> x64:
> | RA == BASE + (RD=NARGS+1)*8 + framesize * 8 +8 > maxstack
> The last summand here is the `LJ_FR2` adjustment.
>
> arm64|mips64 -- incorrect check:
> | RA == BASE + (RD=NARGS)*8 + framesize * 8 >= maxstack
>
Added.
>>       |   add TMP0, RC, #16+FRAME_VARG
>>       |   strLFUNC: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 44fba36c..7f49df5b 100644
>> --- a/src/vm_mips64.dasc
>> +++ b/src/vm_mips64.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
>> new file mode 100644
>> index 00000000..e300d5c1
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
>> @@ -0,0 +1,56 @@
>> +local tap = require('tap')
>> +
>> +-- A test file to demonstrate a stack overflow in `pcall()` in
>> +-- some cases, see below testcase descriptions.
>> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/1048.
>> +local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({
>> +  ['Test requires JIT enabled'] = not jit.status(),
>> +})
>> +
>> +test:plan(2)
>> +
>> +-- The first testcase demonstrate a stack overflow in `pcall()`
>> +-- by recursive calling `pcall()`. The functions are vararg
>> +-- because stack check in BC_IFUNCV is off by one without the
> Minor: by one for the arm64, mips64 architectures.

Updated (here and below):

--- 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
@@ -11,8 +11,8 @@ test:plan(2)

  -- The first testcase demonstrate a stack overflow in `pcall()`
  -- by recursive calling `pcall()`. The functions are vararg
--- because stack check in BC_IFUNCV is off by one without the
--- patch.
+-- because stack check in BC_IFUNCV is off by one on ARM64,
+-- MIPS64 without the patch.
  local function prober_1(...) -- luacheck: no unused
    pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, pairs, {})
  end

>
>> +-- patch.
>> +local function prober_1(...) -- luacheck: no unused
>> +  pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, pairs, {})
>> +end
> Why do we want to use probber_1 here? Why is this different from the
> second example? Only because of the metamethods?
>
> If we want to keep it, please describe why we need at least 9 pcall-s.
As I got right, exactly this number of pcall's is needed to trigger a 
stack overflow.
>
> Also, there is no need for `pairs()` here. Let's use another simpler fast
> function (like `type()`).

(discussed in a private conversation)

Updated:

  local function prober_1(...) -- luacheck: no unused
-  pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, pairs, {})
+  -- Any fast function can be used, but `type` is most convenient
+  -- here because it works fast and can be used with any data type.
+  pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, type, 0)
  end

  local function looper_1(n, ...)

> Also, please add a comment about fast function
> usage, see the example below.
>
>> +
>> +local function looper_1(n, ...)
>> +  prober_1(...)
>> +  prober_1(nil, ...)
> Why do we need `nil` here? I suppose this line is excess, see the
> comment with the example below.

Right, removed:

  end

  local function looper_1(n, ...)
    prober_1(...)
-  prober_1(nil, ...)
    return looper_1(n + 1, n, ...)
  end

>
>> +  return looper_1(n + 1, n, ...)
>> +end
>> +
>> +pcall(coroutine.wrap(looper_1), 0)
>> +
>> +test:ok(true, 'no stack overflow with recursive pcall')
>> +
>> +-- The second testcase demonstrate a stack overflow in `pcall()`
>> +-- with using metamethods. A stack overflow is triggered when
>> +-- `pcall()` is used as `__call` metamethod, setting metatable
>> +-- will cause an infinite loop which fills up the stack with
>> +-- `pcall`-frames and then keeps going beyond the end of the
>> +-- stack until it segfaults.
> This comment is unrelated to this test.

Updated and now it looks as the following:


-- The testcase demonstrate a 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.

>
>>                                 Also, a stack overflow can be
>> +-- triggered when `pcall()` is used as `__newindex` metamethod.
>> +-- The functions are vararg because stack check in BC_IFUNCV is
>> +-- off by one without the patch.
>> +
>> +local mt = setmetatable({}, { __newindex = pcall, __call = pairs })
>> +
>> +local function prober_2(...) -- luacheck: no unused
>> +  mt[mt] = mt
>> +end
>> +
>> +local function looper_2(n, ...)
>> +  prober_2(...)
>> +  prober_2(nil, ...)
>> +  return looper_2(n + 1, n, ...)
>> +end
>> +
>> +pcall(coroutine.wrap(looper_2), 0)
> This can be simplified to the following:
> | src/luajit -e '
> | -- Do not use a Lua function as metamethod -- since it will check
> | -- the stack on each invocation. Use simple `type()` built-in
> | -- instead.
> | local t = setmetatable({}, {__newindex = pcall, __call = type})
> | local function prober(...)
> |     -- Invokes `pcall(t, t, t)`.
> |     t[t] = t
> | end
> | local function looper(n, ...)
> |     prober(...)
> |     return looper(n+1, n, ...)
> | end
> | pcall(coroutine.wrap(looper), 0)
> | '

Updated (added a comment about FF and removed prober() with nil):

@@ -37,15 +38,18 @@ test:ok(true, 'no stack overflow with recursive pcall')
  -- The functions are vararg because stack check in BC_IFUNCV is
  -- off by one without the patch.

-local mt = setmetatable({}, { __newindex = pcall, __call = pairs })
+-- The `type()` function is more convenient here, it works fast
+-- and can be used with any data type. However, any fast function
+-- can be used instead.
+local t = setmetatable({}, { __newindex = pcall, __call = type })

  local function prober_2(...) -- luacheck: no unused
-  mt[mt] = mt
+  -- Invokes `pcall(t, t, t)`.
+  t[t] = t
  end

  local function looper_2(n, ...)
    prober_2(...)
-  prober_2(nil, ...)
    return looper_2(n + 1, n, ...)
  end

>> +
>> +test:ok(true, 'no stack overflow with using metamethod')
>> +
>> +test:done(true)
>> -- 
>> 2.43.0
>>

[-- Attachment #2: Type: text/html, Size: 20866 bytes --]