<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey,</p>
<p>thanks for review! Please consider my comments below.</p>
<p>Sergey</p>
<div class="moz-cite-prefix">On 9/1/25 16:36, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:aLWhRkPqzTnVQqvM@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On 27.08.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Analyzed by Peter Cawley.
(cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
In the commit "LJ_FR2: Fix stack checks in vararg calls."
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Minor: In the previous commit ("...")</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aLWhRkPqzTnVQqvM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">stack overflow in `pcall()`/`xpcall()` was fixed partially and
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
This handles stack overflow for vararg functions and metamethod
invocations not `xpcall()/pcall()` (directly).
</pre>
</blockquote>
<p>Updated:</p>
<p>> In the previous commit ("LJ_FR2: Fix stack checks in vararg
calls.") <br>
> stack overflow for vararg functions and metamethod
invocations <br>
> was fixed partially and there are still cases where stack
overflow <br>
> happens, see comments in the test. The patch fixes the issue
by <br>
> adding the stack check to fast functions `pcall()` and
`xpcall()`. <br>
</p>
<blockquote type="cite" cite="mid:aLWhRkPqzTnVQqvM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">there are still cases where stack overflow happens, see comments
in the test. The patch add stack check to `pcall()` and `xpcall()`.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Please, mention in the commit message that the issue was fixed by adding
the stack check to these fast functions.
</pre>
</blockquote>
Added.
<blockquote type="cite" cite="mid:aLWhRkPqzTnVQqvM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
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, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>
| ldrb RA, [DISPATCH, #DISPATCH_GL(hookmask)]
| cmp <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, #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, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>
| 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, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>
+ | cmp TMP1, TMP2
+ | blo ->fff_fallback
| cmp <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, #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, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>
+ | cmp TMP1, TMP2
+ | blo ->fff_fallback
| ldp CARG1, CARG2, [BASE]
| ldrb TMP0w, GL->hookmask
| subs <a class="moz-txt-link-freetext" href="NARGS8:TMP1">NARGS8:TMP1</a>, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, #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, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>
| lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
| beqz <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, ->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 <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, -8
|
|.ffunc xpcall
+ | lw TMP1, L->maxstack
+ | addu TMP2, BASE, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>
| sltiu AT, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, 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, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>
+ | sltu AT, TMP1, TMP2
+ | bnez AT, ->fff_fallback
+ |. lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
| daddiu <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, -8
- | lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
| bltz <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, ->fff_fallback
|. move TMP2, BASE
| daddiu BASE, BASE, 16
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.\</pre>
</blockquote>
Done.
<blockquote type="cite" cite="mid:aLWhRkPqzTnVQqvM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">@@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)
|. nop
|
|.ffunc xpcall
+ | ld TMP1, L->maxstack
+ | daddu TMP2, BASE, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>
+ | sltu AT, TMP1, TMP2
+ | bnez AT, ->fff_fallback
+ |. ld CARG1, 0(BASE)
| daddiu <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, -16
- | ld CARG1, 0(BASE)
| ld CARG2, 8(BASE)
| bltz <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, ->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, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>
| cmplwi <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, 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, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>
| cmplwi <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, 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 <a class="moz-txt-link-freetext" href="L:RB">L:RB</a>, SAVE_L
+ | lea RA, [<a class="moz-txt-link-freetext" href="BASE+NARGS:RD*8">BASE+NARGS:RD*8</a>]
+ | cmp RA, <a class="moz-txt-link-freetext" href="L:RB">L:RB</a>->maxstack; ja ->fff_fallback
| lea RA, [BASE+16]
| sub <a class="moz-txt-link-freetext" href="NARGS:RDd">NARGS:RDd</a>, 1
| mov PCd, 16+FRAME_PCALL
@@ -1563,6 +1566,9 @@ static void build_subroutines(BuildCtx *ctx)
| jmp ->vm_call_dispatch
|
|.ffunc_2 xpcall
+ | mov <a class="moz-txt-link-freetext" href="L:RB">L:RB</a>, SAVE_L
+ | lea RA, [<a class="moz-txt-link-freetext" href="BASE+NARGS:RD*8">BASE+NARGS:RD*8</a>]
+ | cmp RA, <a class="moz-txt-link-freetext" href="L:RB">L:RB</a>->maxstack; ja ->fff_fallback
| mov <a class="moz-txt-link-freetext" href="LFUNC:RA">LFUNC:RA</a>, [BASE+8]
| checktp_nc <a class="moz-txt-link-freetext" href="LFUNC:RA">LFUNC:RA</a>, LJ_TFUNC, ->fff_fallback
| mov <a class="moz-txt-link-freetext" href="LFUNC:RB">LFUNC:RB</a>, [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 <a class="moz-txt-link-freetext" href="L:RB">L:RB</a>, SAVE_L
+ | lea RA, [<a class="moz-txt-link-freetext" href="BASE+NARGS:RD*8">BASE+NARGS:RD*8</a>]
+ | cmp RA, <a class="moz-txt-link-freetext" href="L:RB">L:RB</a>->maxstack; ja ->fff_fallback
| lea RA, [BASE+8]
| sub <a class="moz-txt-link-freetext" href="NARGS:RD">NARGS:RD</a>, 1
| mov PC, 8+FRAME_PCALL
@@ -1925,6 +1928,9 @@ static void build_subroutines(BuildCtx *ctx)
| jmp ->vm_call_dispatch
|
|.ffunc_2 xpcall
+ | mov <a class="moz-txt-link-freetext" href="L:RB">L:RB</a>, SAVE_L
+ | lea RA, [<a class="moz-txt-link-freetext" href="BASE+NARGS:RD*8">BASE+NARGS:RD*8</a>]
+ | cmp RA, <a class="moz-txt-link-freetext" href="L:RB">L:RB</a>->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)
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with using metamethod')
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.</pre>
</blockquote>
<p>this testcase is failed too:</p>
<p>TAP version 13
<br>
1..4
<br>
ok - no stack overflow with recursive pcall
<br>
ok - no stack overflow with metamethod
<br>
=================================================================
<br>
==3374==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x007f77c03fc8 at pc 0x005574fc4fb0 bp 0x007ff2e0f3f0<br>
sp 0x007ff2e0f410
<br>
WRITE of size 8 at 0x007f77c03fc8 thread T0
<br>
#0 0x5574fc4faf in copyTV
/root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="obj.h:1000">obj.h:1000</a> <br>
#1 0x5574fc4faf in lj_meta_call
/root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="meta.c:444">meta.c:444</a><br>
#2 0x557510c293 in lj_vmeta_call
(/root/sergeyb/luajit/build/src/luajit+0x1d5293)<br>
#3 0x5574f92a9b in lua_pcall
/root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="api.c:1173">api.c:1173</a> <br>
#4 0x5574f74217 in docall
/root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:134">luajit.c:134</a><br>
#5 0x5574f75067 in handle_script
/root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:304">luajit.c:304</a> <br>
#6 0x5574f76d07 in pmain
/root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:602">luajit.c:602</a> <br>
#7 0x557510bb33 in lj_BC_FUNCC
(/root/sergeyb/luajit/build/src/luajit+0x1d4b33)<br>
#8 0x5574f937fb in lua_cpcall
/root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="api.c:1208">api.c:1208</a> <br>
#9 0x5574f77083 in main /root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:633">luajit.c:633</a><br>
#10 0x7f7dbfe79f in __libc_start_main
(/lib/aarch64-linux-gnu/libc.so.6+0x2079f)<br>
#11 0x5574f73aa3
(/root/sergeyb/luajit/build/src/luajit+0x3caa3)<br>
<br>
</p>
<blockquote type="cite" cite="mid:aLWhRkPqzTnVQqvM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- 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, {})'
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Why do we need this test if it has the same semantics as the second
one?</pre>
</blockquote>
the test was removed
<blockquote type="cite" cite="mid:aLWhRkPqzTnVQqvM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ 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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.</pre>
</blockquote>
Added.
<blockquote type="cite" cite="mid:aLWhRkPqzTnVQqvM@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ t[i], t[i + 1], t[i + 2] = pcall, pairs, {}
+ coroutine.wrap(f)()
+end
+
+test:ok(true, 'no stack overflow with unpacked pcalls')
+
<a class="moz-txt-link-freetext" href="test:done(true)">test:done(true)</a>
--
2.43.0
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>