<!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!</p>
<div class="moz-cite-prefix">On 2/11/26 11:30, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">On 10.12.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">From: Mike Pall <mike>
Thanks to Peter Cawley.
(cherry picked from commit d1a2fef8a8f53b0055ee041f7f63d83a27444ffa)
Stack overflow can cause a segmentation fault in vararg
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/vararg/a vararg/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">function on ARM64 and MIPS64 in LJ_FR2 mode. This happen
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/happen/happens/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">because stack check in BC_IFUNCV is off by one on these
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/stack/the stack/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">platforms without the patch. The original stack check
for ARM64 and MIPS64 was incorrect:
| RA == BASE + (RD=NARGS)*8 + framesize * 8 >= maxstack
while stack check on x86_64 is correct and therefore is
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/stack/the stack/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">not affected by the problem:
| RA == BASE + (RD=NARGS+1)*8 + framesize * 8 +8 > maxstack
The patch partially fixes aforementioned issue by
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/aforementioned/the aforementioned/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">bumping LJ_STACK_EXTRA by 1 to give a space to write
the entire frame link and fixing a number of last
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/a number/the number/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
I'm not get this part. I suggest rephrasing it like the following:
| The patch partially fixes the aforementioned issue by bumping
| LJ_STACK_EXTRA by 1 to give a space to the entire frame link for a
| vararg function as the __newindex metamethod.</pre>
</blockquote>
Updated.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">free slot in the stack (LJ_FR2 summand adjustment).
A fixup for a number of required slots in `call_init()` was added
for consistency with non-gc64 flavor.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/gc64/GC64/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
I would also add: "The check is too strict, so this can't lead to any
crash."
</pre>
</blockquote>
Updated.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@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#12134
---
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 | 53 +++++++++++++++++++
5 files changed, 57 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;
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Let's add an additional test for this part of code (since we don't have
any). It may be taken from [1]. It doesn't fail now, but we may cover
this branch more precise.
</pre>
</blockquote>
<p>Don't get what do you mean. </p>
<p>true branch in gc32 is covered by the following tests:</p>
<p>test/LuaJIT-tests<br>
test/PUC-Rio-Lua-5.1-tests<br>
test/tarantool-c-tests/lj-1087-vm-handler-call.c_test<br>
test/tarantool-tests/fix-ff-select-recording.test.lua<br>
test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua<br>
test/tarantool-tests/fix-slot-check-for-mm-record.test.lua<br>
test/tarantool-tests/fix-slots-overflow-for-varg-record.test.lua<br>
test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua<br>
test/tarantool-tests/lj-1024-varg-maxslot.test.lua<br>
test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua<br>
test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua<br>
test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua<br>
test/tarantool-tests/lj-1164-record-meta-concat-varg-pcall.test.lua<br>
test/tarantool-tests/lj-1295-bad-renames-for-sunk-values.test.lua<br>
test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua<br>
test/tarantool-tests/lj-704-bc-varg-use-def.test.lua<br>
<br>
</p>
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
index da187a7a..6c2975b4 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..d471d41e
--- /dev/null
+++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
@@ -0,0 +1,53 @@
+local tap = require('tap')
+
+-- A test file to demonstrate a stack overflow in `pcall()` in
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
I would rephrase it like "demonstrate a crash due to Lua stack
out-of-bounds access".</pre>
</blockquote>
<p>Updated:</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>
@@ -1,7 +1,7 @@<br>
local tap = require('tap')<br>
<br>
--- A test file to demonstrate a stack overflow in `pcall()` in<br>
--- some cases, see below testcase descriptions.<br>
+-- A test file to demonstrate a crash due to Lua stack<br>
+-- out-of-bounds access, see below testcase descriptions.<br>
-- See also <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1048">https://github.com/LuaJIT/LuaJIT/issues/1048</a>.<br>
local test =
tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({<br>
['Test requires JIT enabled'] = not jit.status(),<br>
<br>
</p>
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- 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(),
+})
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
I suppose there is no need in JIT here. This skipcond may be removed.
</pre>
</blockquote>
Removed.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+test:plan(2)
+
+-- The testcase demonstrate a segmentation fault due to stack
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/testcase demonstrate/test case demonstrates/</pre>
</blockquote>
Updated.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- overflow by recursive calling `pcall()`. The functions are
+-- vararg because stack check in BC_IFUNCV is off by one on ARM64
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/stack/the stack/</pre>
</blockquote>
Updated.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- and MIPS64 without the patch.
+local function prober_1(...) -- luacheck: no unused
+ -- Any fast function can be used as metamethod, but `type` is
+ -- convenient here because it works fast and can be used with
+ -- any data type. Lua function cannot be used since it
+ -- will check the stack on each invocation.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Please add the comment, that we need to check using of the correct
value LJ_STACK_EXTRA slots (5+3*LJ_FR2) = 8 for GC64 mode.</pre>
</blockquote>
<p>Updated:</p>
<p> local function prober_1(...) -- luacheck: no unused<br>
-- Any fast function can be used as metamethod, but `type` is<br>
-- convenient here because it works fast and can be used with<br>
-- any data type. Lua function cannot be used since it<br>
- -- will check the stack on each invocation.<br>
+ -- will check the stack on each invocation. We need to check<br>
+ -- using of the correct value LJ_STACK_EXTRA slots<br>
+ -- (5+3*LJ_FR2) = 8 for GC64 mode.<br>
pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall,
type, 0)<br>
end<br>
<br>
<br>
</p>
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, type, 0)
+end
+
+local function looper(prober, n, ...)
+ prober(...)
+ return looper(prober, n + 1, n, ...)
+end
+
+pcall(coroutine.wrap(looper), prober_1, 0)
+
+test:ok(true, 'no stack overflow with recursive pcall')
+
+-- The testcase demonstrate a segmentation fault due to stack
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/testcase demonstrate/test case demonstrates/</pre>
</blockquote>
Updated.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- overflow when `pcall()` is used as `__newindex` metamethod.
+-- The function is vararg because stack check in BC_IFUNCV is off
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/stack/the stack/</pre>
</blockquote>
Updated.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- by one on ARM64 and MIPS64 without the patch.
+
+-- Any fast function can be used as metamethod, but `type` is
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/metamethod/a metamethod/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- convenient here because it works fast and can be used with
+-- any data type. Lua function cannot be used since it
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/Lua/The Lua/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYw-E0RBGoaZ-w_s@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- will check the stack on each invocation.
+local t = setmetatable({}, { __newindex = pcall, __call = type })
+
+local function prober_2(...) -- luacheck: no unused
+ -- Invokes `pcall(t, t, t)`.
+ t[t] = t
+end
+
+pcall(coroutine.wrap(looper), prober_2, 0)
+
+test:ok(true, 'no stack overflow with metamethod')
+
+test:done(true)
--
2.43.0
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
[1]: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1402#issue-3569942423">https://github.com/LuaJIT/LuaJIT/issues/1402#issue-3569942423</a>
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>