<!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 see my comments below.</p>
<p>Sergey</p>
<div class="moz-cite-prefix">On 9/1/25 16:07, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@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">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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
This is unrelated to this patch, so it can be omitted.</pre>
</blockquote>
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
This issue is not related to this patch.
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Either of these points can cause an issue if `pcall()` is used as
`__newindex`.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Looks like the metamethods are not required for issue reproducing.
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> The patch partially fixes aforementioned issues.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
By how?</pre>
</blockquote>
<p>I've updated the commit message as the following:</p>
<p> Stack overflow can cause a segmentation fault in vararg<br>
function on ARM64 and MIPS64 in LJ_FR2 mode. This happen<br>
because stack check in BC_IFUNCV is off by one on these<br>
platforms without the patch. The patch partially fixes<br>
aforementioned issue by bumping LJ_STACK_EXTRA by 1 to<br>
give a space to write the entire frame link and fixing<br>
a number of last free slot in the stack.<br>
<br>
</p>
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@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/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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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;
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.</pre>
</blockquote>
<p>Please check again. Both testcases trigger segfault on AArch64
(odroid).</p>
<p>cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON
-DLUA_USE_APICHECK=ON</p>
<p>cmake --build build --parallel</p>
<p>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"</p>
<p>(gdb) bt</p>
#0 0x00000055555c16f4 in lj_alloc_free (msp=0x7fb7d56010,
ptr=0x7fb7d69088)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="alloc.c:1405">alloc.c:1405</a><br>
#1 0x00000055555c1fe4 in lj_alloc_realloc (msp=0x7fb7d56010,
ptr=0x7fb7d69088, nsize=1696)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="alloc.c:1471">alloc.c:1471</a><br>
#2 0x00000055555c204c in lj_alloc_f (msp=0x7fb7d56010,
ptr=0x7fb7d69088, osize=816, nsize=1696)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="alloc.c:1486">alloc.c:1486</a><br>
#3 0x00000055555790e0 in lj_mem_realloc (L=0x7fb7d6d330,
p=0x7fb7d69088, osz=816, nsz=1696)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="gc.c:896">gc.c:896</a><br>
#4 0x000000555557e610 in resizestack (L=0x7fb7d6d330, n=204) at
/root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="state.c:82">state.c:82</a><br>
#5 0x000000555557e970 in lj_state_growstack (L=0x7fb7d6d330,
need=48)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="state.c:130">state.c:130</a><br>
#6 0x00000055555fad68 in lj_vm_growstack_l () at
buildvm_<a class="moz-txt-link-freetext" href="arm64.dasc:1263">arm64.dasc:1263</a><br>
#7 0x00000055555fb8d4 in lj_ff_coroutine_wrap_aux () at
buildvm_<a class="moz-txt-link-freetext" href="arm64.dasc:1775">arm64.dasc:1775</a><br>
#8 0x000000555556a824 in lua_pcall (L=0x7fb7d56378, nargs=0,
nresults=-1, errfunc=2)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="api.c:1173">api.c:1173</a><br>
#9 0x000000555555d258 in docall (L=0x7fb7d56378, narg=0, clear=0)
at /root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:134">luajit.c:134</a><br>
#10 0x000000555555db9c in handle_script (L=0x7fb7d56378,
argx=0x7ffffff280)<br>
at /root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:304">luajit.c:304</a><br>
#11 0x000000555555ea54 in pmain (L=0x7fb7d56378) at
/root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:602">luajit.c:602</a><br>
#12 0x00000055555fab90 in lj_BC_FUNCC () at buildvm_<a class="moz-txt-link-freetext" href="arm64.dasc:894">arm64.dasc:894</a><br>
#13 0x000000555556ad90 in lua_cpcall (L=0x7fb7d56378,
func=0x555555e898 <pmain>, ud=0x0)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="api.c:1208">api.c:1208</a><br>
#14 0x000000555555ebb4 in main (argc=4, argv=0x7ffffff268) at
/root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:633">luajit.c:633</a><br>
<p>(gdb)</p>
<p>With commented out first testcase:</p>
<p>(gdb) bt<br>
#0 0x00000055555c18fc in lj_alloc_free (msp=0x7fb7d56010,
ptr=0x7fb7d69068)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="alloc.c:1406">alloc.c:1406</a><br>
#1 0x00000055555c1fe4 in lj_alloc_realloc (msp=0x7fb7d56010,
ptr=0x7fb7d69068, nsize=1696)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="alloc.c:1471">alloc.c:1471</a><br>
#2 0x00000055555c204c in lj_alloc_f (msp=0x7fb7d56010,
ptr=0x7fb7d69068, osize=816, nsize=1696)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="alloc.c:1486">alloc.c:1486</a><br>
#3 0x00000055555790e0 in lj_mem_realloc (L=0x7fb7d6d2a0,
p=0x7fb7d69068, osz=816, nsz=1696)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="gc.c:896">gc.c:896</a><br>
#4 0x000000555557e610 in resizestack (L=0x7fb7d6d2a0, n=204) at
/root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="state.c:82">state.c:82</a><br>
#5 0x000000555557e970 in lj_state_growstack (L=0x7fb7d6d2a0,
need=48)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="state.c:130">state.c:130</a><br>
#6 0x00000055555fad68 in lj_vm_growstack_l () at
buildvm_<a class="moz-txt-link-freetext" href="arm64.dasc:1263">arm64.dasc:1263</a><br>
#7 0x00000055555fb8d4 in lj_ff_coroutine_wrap_aux () at
buildvm_<a class="moz-txt-link-freetext" href="arm64.dasc:1775">arm64.dasc:1775</a><br>
#8 0x000000555556a824 in lua_pcall (L=0x7fb7d56378, nargs=0,
nresults=-1, errfunc=2)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="api.c:1173">api.c:1173</a><br>
#9 0x000000555555d258 in docall (L=0x7fb7d56378, narg=0, clear=0)
at /root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:134">luajit.c:134</a><br>
#10 0x000000555555db9c in handle_script (L=0x7fb7d56378,
argx=0x7ffffff280)<br>
at /root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:304">luajit.c:304</a><br>
#11 0x000000555555ea54 in pmain (L=0x7fb7d56378) at
/root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:602">luajit.c:602</a><br>
#12 0x00000055555fab90 in lj_BC_FUNCC () at buildvm_<a class="moz-txt-link-freetext" href="arm64.dasc:894">arm64.dasc:894</a><br>
#13 0x000000555556ad90 in lua_cpcall (L=0x7fb7d56378,
func=0x555555e898 <pmain>, ud=0x0)<br>
at /root/sergeyb/luajit/src/lj_<a class="moz-txt-link-freetext" href="api.c:1208">api.c:1208</a><br>
#14 0x000000555555ebb4 in main (argc=4, argv=0x7ffffff268) at
/root/sergeyb/luajit/src/<a class="moz-txt-link-freetext" href="luajit.c:633">luajit.c:633</a><br>
(gdb)</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> 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 <a class="moz-txt-link-freetext" href="LFUNC:CARG3">LFUNC:CARG3</a>, CARG3, TMP0, lsl #47
| add RA, RA, RC
+ | sub CARG1, CARG1, #8
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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
</pre>
</blockquote>
Added.
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> | add TMP0, RC, #16+FRAME_VARG
| str <a class="moz-txt-link-freetext" href="LFUNC:CARG3">LFUNC:CARG3</a>, [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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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 <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1048">https://github.com/LuaJIT/LuaJIT/issues/1048</a>.
+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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Minor: by one for the arm64, mips64 architectures.</pre>
</blockquote>
<p>Updated (here and below):</p>
<p>---
a/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua<br>
+++
b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua<br>
@@ -11,8 +11,8 @@ <a class="moz-txt-link-freetext" href="test:plan(2)">test:plan(2)</a><br>
<br>
-- The first testcase demonstrate a stack overflow in `pcall()`<br>
-- by recursive calling `pcall()`. The functions are vararg<br>
--- because stack check in BC_IFUNCV is off by one without the<br>
--- patch.<br>
+-- because stack check in BC_IFUNCV is off by one on ARM64,<br>
+-- MIPS64 without the patch.<br>
local function prober_1(...) -- luacheck: no unused<br>
pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall,
pairs, {})<br>
end<br>
<br>
</p>
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- patch.
+local function prober_1(...) -- luacheck: no unused
+ pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, pairs, {})
+end
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.</pre>
</blockquote>
As I got right, exactly this number of pcall's is needed to trigger
a stack overflow.
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@root">
<pre wrap="" class="moz-quote-pre">
Also, there is no need for `pairs()` here. Let's use another simpler fast
function (like `type()`).</pre>
</blockquote>
<p>(discussed in a private conversation)</p>
<p>Updated:</p>
<p> local function prober_1(...) -- luacheck: no unused<br>
- pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall,
pairs, {})<br>
+ -- Any fast function can be used, but `type` is most convenient<br>
+ -- here because it works fast and can be used with any data
type.<br>
+ pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall,
type, 0)<br>
end<br>
<br>
local function looper_1(n, ...)<br>
<br>
</p>
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@root">
<pre wrap="" class="moz-quote-pre">Also, please add a comment about fast function
usage, see the example below.
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+local function looper_1(n, ...)
+ prober_1(...)
+ prober_1(nil, ...)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Why do we need `nil` here? I suppose this line is excess, see the
comment with the example below.</pre>
</blockquote>
<p>Right, removed:</p>
<p> end<br>
<br>
local function looper_1(n, ...)<br>
prober_1(...)<br>
- prober_1(nil, ...)<br>
return looper_1(n + 1, n, ...)<br>
end<br>
</p>
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ 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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
This comment is unrelated to this test.</pre>
</blockquote>
<p>Updated and now it looks as the following:</p>
<p><br>
-- The testcase demonstrate a stack overflow when `pcall()`<br>
-- is used as `__newindex` metamethod. The function is vararg<br>
-- because stack check in BC_IFUNCV is off by one on ARM64<br>
-- and MIPS64 without the patch.<br>
<br>
</p>
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> 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)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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)
| '
</pre>
</blockquote>
<p>Updated (added a comment about FF and removed prober() with nil):</p>
<p>@@ -37,15 +38,18 @@ <a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with
recursive pcall')<br>
-- The functions are vararg because stack check in BC_IFUNCV is<br>
-- off by one without the patch.<br>
<br>
-local mt = setmetatable({}, { __newindex = pcall, __call = pairs
})<br>
+-- The `type()` function is more convenient here, it works fast<br>
+-- and can be used with any data type. However, any fast function<br>
+-- can be used instead.<br>
+local t = setmetatable({}, { __newindex = pcall, __call = type })<br>
<br>
local function prober_2(...) -- luacheck: no unused<br>
- mt[mt] = mt<br>
+ -- Invokes `pcall(t, t, t)`.<br>
+ t[t] = t<br>
end<br>
<br>
local function looper_2(n, ...)<br>
prober_2(...)<br>
- prober_2(nil, ...)<br>
return looper_2(n + 1, n, ...)<br>
end<br>
<br>
</p>
<blockquote type="cite" cite="mid:aLWaqnSxhdwxR7DL@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+test:ok(true, 'no stack overflow with using metamethod')
+
+test:done(true)
--
2.43.0
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>