[Tarantool-patches] [PATCH luajit 2/2] Add stack check to pcall/xpcall.
Sergey Kaplun
skaplun at tarantool.org
Mon Sep 1 16:36:06 MSK 2025
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
More information about the Tarantool-patches
mailing list