* [Tarantool-patches] [PATCH luajit 0/2] Fix stack overflow in pcall/xpcall
@ 2025-08-27 9:44 Sergey Bronnikov via Tarantool-patches
2025-08-27 9:44 ` [Tarantool-patches] [PATCH luajit 1/2] LJ_FR2: Fix stack checks in vararg calls Sergey Bronnikov via Tarantool-patches
2025-08-27 9:44 ` [Tarantool-patches] [PATCH luajit 2/2] Add stack check to pcall/xpcall Sergey Bronnikov via Tarantool-patches
0 siblings, 2 replies; 6+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2025-08-27 9:44 UTC (permalink / raw)
To: tarantool-patches, Sergey Kaplun
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH luajit 1/2] LJ_FR2: Fix stack checks in vararg calls.
2025-08-27 9:44 [Tarantool-patches] [PATCH luajit 0/2] Fix stack overflow in pcall/xpcall Sergey Bronnikov via Tarantool-patches
@ 2025-08-27 9:44 ` Sergey Bronnikov via Tarantool-patches
2025-09-01 13:07 ` Sergey Kaplun via Tarantool-patches
2025-08-27 9:44 ` [Tarantool-patches] [PATCH luajit 2/2] Add stack check to pcall/xpcall Sergey Bronnikov via Tarantool-patches
1 sibling, 1 reply; 6+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2025-08-27 9:44 UTC (permalink / raw)
To: tarantool-patches, Sergey Kaplun
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH luajit 2/2] Add stack check to pcall/xpcall.
2025-08-27 9:44 [Tarantool-patches] [PATCH luajit 0/2] Fix stack overflow in pcall/xpcall Sergey Bronnikov via Tarantool-patches
2025-08-27 9:44 ` [Tarantool-patches] [PATCH luajit 1/2] LJ_FR2: Fix stack checks in vararg calls Sergey Bronnikov via Tarantool-patches
@ 2025-08-27 9:44 ` Sergey Bronnikov via Tarantool-patches
2025-09-01 13:36 ` Sergey Kaplun via Tarantool-patches
1 sibling, 1 reply; 6+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2025-08-27 9:44 UTC (permalink / raw)
To: tarantool-patches, Sergey Kaplun
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] LJ_FR2: Fix stack checks in vararg calls.
2025-08-27 9:44 ` [Tarantool-patches] [PATCH luajit 1/2] LJ_FR2: Fix stack checks in vararg calls Sergey Bronnikov via Tarantool-patches
@ 2025-09-01 13:07 ` Sergey Kaplun via Tarantool-patches
2025-09-23 17:49 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 1 reply; 6+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2025-09-01 13:07 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] Add stack check to pcall/xpcall.
2025-08-27 9:44 ` [Tarantool-patches] [PATCH luajit 2/2] Add stack check to pcall/xpcall Sergey Bronnikov via Tarantool-patches
@ 2025-09-01 13:36 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2025-09-01 13:36 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] LJ_FR2: Fix stack checks in vararg calls.
2025-09-01 13:07 ` Sergey Kaplun via Tarantool-patches
@ 2025-09-23 17:49 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2025-09-23 17:49 UTC (permalink / raw)
To: Sergey Kaplun, Sergey Bronnikov; +Cc: 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-09-23 17:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-08-27 9:44 [Tarantool-patches] [PATCH luajit 0/2] Fix stack overflow in pcall/xpcall Sergey Bronnikov via Tarantool-patches
2025-08-27 9:44 ` [Tarantool-patches] [PATCH luajit 1/2] LJ_FR2: Fix stack checks in vararg calls Sergey Bronnikov via Tarantool-patches
2025-09-01 13:07 ` Sergey Kaplun via Tarantool-patches
2025-09-23 17:49 ` Sergey Bronnikov via Tarantool-patches
2025-08-27 9:44 ` [Tarantool-patches] [PATCH luajit 2/2] Add stack check to pcall/xpcall Sergey Bronnikov via Tarantool-patches
2025-09-01 13:36 ` Sergey Kaplun via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox