<!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>I've updated N_ITERATIONS and force-pushed the branch.</p>
<p>Sergey</p>
<div class="moz-cite-prefix">On 3/12/26 20:19, Sergey Kaplun wrote:<br>
</div>
<blockquote type="cite" cite="mid:abL1rBy6MT8uxOcW@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing the last nit below.
On 12.03.26, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Hi, Sergey,
thanks for review! See my comments below.
Sergey
On 3/12/26 13:16, Sergey Kaplun via Tarantool-patches wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the patch!
Please, fix my comments below.
Don't forget to add the corresponding iterative changes.
On 12.03.26, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">From: Mike Pall <mike>
Analyzed by Peter Cawley.
(cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
The patch adds the stack check to fast functions `pcall()` and
`xpcall()`.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">Please add more verbose description:
| (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
|
| The `pcall()` and `xpcall()` calls in GC64 mode require 2 slots. This
| means that all arguments should be moved up during emitting of the frame
| link to the stack. Hence, this may cause stack overflow without the
| corresponding check.
|
| This patch adds the corresponding checks to the VM. Non-GC64 VMs are
| updated as well for the consistency.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">Updated
</pre>
<blockquote type="cite">
<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/vm_arm.dasc | 7 ++++
src/vm_arm64.dasc | 8 +++++
src/vm_mips.dasc | 10 +++++-
src/vm_mips64.dasc | 14 ++++++--
src/vm_ppc.dasc | 9 +++++
src/vm_x64.dasc | 6 ++++
src/vm_x86.dasc | 6 ++++
...048-fix-stack-checks-vararg-calls.test.lua | 35 ++++++++++++++++++-
8 files changed, 90 insertions(+), 5 deletions(-)
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
index 6c2975b4..4e60ee07 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)
| <a class="moz-txt-link-freetext" href="daddiuNARGS8:RC,NARGS8:RC">daddiuNARGS8:RC,NARGS8:RC</a>, -8
- | lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
| <a class="moz-txt-link-freetext" href="bltzNARGS8:RC">bltzNARGS8:RC</a>, ->fff_fallback
|. move TMP2, BASE
| daddiu BASE, BASE, 16
@@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)
|. nop
|
|.ffunc xpcall
- | <a class="moz-txt-link-freetext" href="daddiuNARGS8:TMP0,NARGS8:RC">daddiuNARGS8:TMP0,NARGS8:RC</a>, -16
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">This neglets the first patch in the series. See the comment below.
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">- | ld CARG1, 0(BASE)
+ | 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)
+ | <a class="moz-txt-link-freetext" href="daddiuNARGS8:RC,NARGS8:RC">daddiuNARGS8:RC,NARGS8:RC</a>, -16
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">This line is incorrect. This neglets the 1st patch in the series.
It should be
| | <a class="moz-txt-link-freetext" href="daddiuNARGS8:TMP0,NARGS8:RC">daddiuNARGS8:TMP0,NARGS8:RC</a>, -16
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Right. However, probably we should leave this line near ".ffunc xpcall".
What do you think?
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Why? This break the `maxstack` check (since the RC is differs before the
addition with TMP2).</pre>
</blockquote>
<p>Ok, let's leave as is, seems I took a look on intermediate
version, before applying changes you requested on review.</p>
<p>The current patch looks fine for me:</p>
<p>@@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx
*ctx)<br>
|. nop<br>
|<br>
|.ffunc xpcall<br>
+ | ld TMP1, L->maxstack<br>
+ | daddu TMP2, BASE, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a><br>
+ | sltu AT, TMP1, TMP2<br>
+ | bnez AT, ->fff_fallback<br>
+ |. ld CARG1, 0(BASE)<br>
| daddiu <a class="moz-txt-link-freetext" href="NARGS8:TMP0">NARGS8:TMP0</a>, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, -16<br>
- | ld CARG1, 0(BASE)<br>
| ld CARG2, 8(BASE)<br>
| bltz <a class="moz-txt-link-freetext" href="NARGS8:TMP0">NARGS8:TMP0</a>, ->fff_fallback<br>
|. lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)<br>
<br>
</p>
<blockquote type="cite" cite="mid:abL1rBy6MT8uxOcW@root">
<pre wrap="" class="moz-quote-pre">
See the latest LuaJIT version:
<a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/blob/659a61693aa3b87661864ad0f12eee14c865cd7f/src/vm_mips64.dasc#L1450">https://github.com/LuaJIT/LuaJIT/blob/659a61693aa3b87661864ad0f12eee14c865cd7f/src/vm_mips64.dasc#L1450</a>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
Now updated as the following:
--- a/src/vm_mips64.dasc
+++ b/src/vm_mips64.dasc
@@ -1449,7 +1449,7 @@ static void build_subroutines(BuildCtx *ctx)
| 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
+ | daddiu <a class="moz-txt-link-freetext" href="NARGS8:TMP0">NARGS8:TMP0</a>, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, -16
| ld CARG2, 8(BASE)
| bltz <a class="moz-txt-link-freetext" href="NARGS8:TMP0">NARGS8:TMP0</a>, ->fff_fallback
|. lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> | ld CARG2, 8(BASE)
| <a class="moz-txt-link-freetext" href="bltzNARGS8:TMP0">bltzNARGS8:TMP0</a>, ->fff_fallback
|. lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<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
index 3a8ad63d..ad8b151b 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
@@ -5,7 +5,7 @@ local tap = require('tap')
-- See <a class="moz-txt-link-freetext" href="alsohttps://github.com/LuaJIT/LuaJIT/issues/1048">alsohttps://github.com/LuaJIT/LuaJIT/issues/1048</a>.
local test = tap.test('lj-1048-fix-stack-checks-vararg-calls')
-test:plan(2)
+test:plan(5)
-- The test case demonstrates a segmentation fault due to stack
-- overflow by recursive calling `pcall()`. The functions are
@@ -50,4 +50,37 @@ pcall(coroutine.wrap(looper), prober_2, 0)
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with metamethod')
+-- The testcases demonstrates a stack overflow in
+-- `pcall()`/xpcall()` triggered using metamethod `__call`.
+
+t = coroutine.wrap(setmetatable)({}, { __call = pcall })
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">I've meant the following:
| t = setmetatable({}, { __call = pcall })
| coroutine.wrap(function() t() end)()
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">Updated
@@ -53,7 +53,8 @@ <a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with metamethod')
-- The testcases demonstrates a stack overflow in
-- `pcall()`/xpcall()` triggered using metamethod `__call`.
-t = coroutine.wrap(setmetatable)({}, { __call = pcall })
+t = setmetatable({}, { __call = pcall })
+coroutine.wrap(function() t() end)()
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with metamethod __call with pcall()')
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+test:ok(true, 'no stack overflow with metamethod __call with pcall()')
+
+t = coroutine.wrap(setmetatable)({}, { __call = xpcall })
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">I've meant the following:
| t = setmetatable({}, { __call = xpcall })
| coroutine.wrap(function() t() end)()
But this won't work since the second amount of xpcall must be the
function. So, this test case is invalid. We must to duplicate the second
approach with `xpcall()`
This works fine.
| LUA_PATH="src/?.lua;;" gdb --args src/luajit -e '
| local t = {}
| local function xpcall_wrapper()
| return xpcall(unpack(t))
| end
|
| local N_ITERATIONS = 200
|
| for i = 1, N_ITERATIONS do
| t[i], t[i + 1], t[i + 2] = xpcall, type, {}
| coroutine.wrap(xpcall_wrapper)()
| end
| '
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Updated:
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 6395dfaa..825568f9 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
@@ -58,7 +58,17 @@ coroutine.wrap(function() t() end)()
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with metamethod __call with pcall()')
-t = coroutine.wrap(setmetatable)({}, { __call = xpcall })
+t = {}
+local function xpcall_wrapper()
+ return xpcall(unpack(t))
+end
+
+local N_ITERATIONS_1 = 200
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Why do we need two variables with the same value of iterations?
Let's use N_ITERATIONS with the comment for xpcall and pcall.</pre>
</blockquote>
<p>Updated:</p>
<p>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<br>
index 825568f9..036d53e9 100644<br>
---
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>
@@ -63,9 +63,13 @@ local function xpcall_wrapper()<br>
return xpcall(unpack(t))<br>
end<br>
<br>
-local N_ITERATIONS_1 = 200<br>
+-- The problems are only reproduced on LuaJIT GC64 and is better<br>
+-- reproduced under Valgrind than AddressSanitizer. The chosen<br>
+-- value was found experimentally and always results in an
attempt<br>
+-- to write beyond the allocated memory.<br>
+local N_ITERATIONS = 200<br>
<br>
-for i = 1, N_ITERATIONS_1 do<br>
+for i = 1, N_ITERATIONS do<br>
t[i], t[i + 1], t[i + 2] = xpcall, type, {}<br>
coroutine.wrap(xpcall_wrapper)()<br>
end<br>
@@ -81,13 +85,7 @@ local function pcall_wrapper()<br>
return pcall(unpack(t))<br>
end<br>
<br>
--- The problem is only reproduced on LuaJIT GC64 and is better<br>
--- reproduced under Valgrind than AddressSanitizer. The chosen<br>
--- value was found experimentally and always results in an
attempt<br>
--- to write beyond the allocated memory.<br>
-local N_ITERATIONS_2 = 200<br>
-<br>
-for i = 1, N_ITERATIONS_2 do<br>
+for i = 1, N_ITERATIONS do<br>
t[i], t[i + 1], t[i + 2] = pcall, type, {}<br>
coroutine.wrap(pcall_wrapper)()<br>
end<br>
<br>
</p>
<blockquote type="cite" cite="mid:abL1rBy6MT8uxOcW@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+for i = 1, N_ITERATIONS_1 do
+ t[i], t[i + 1], t[i + 2] = xpcall, type, {}
+ coroutine.wrap(xpcall_wrapper)()
+end
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with metamethod __call with xpcall()')
@@ -67,19 +77,19 @@ <a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with metamethod
__call with xpcall()')
-- triggered using `unpack()`.
t = {}
-local function f()
+local function pcall_wrapper()
return pcall(unpack(t))
end
--- The problem is only reproduced on LuaJIT GC64 and is best
+-- The problem is only reproduced on LuaJIT GC64 and is better
-- reproduced under Valgrind than AddressSanitizer. The chosen
-- value was found experimentally and always results in an attempt
-- to write beyond the allocated memory.
-local N_ITERATIONS = 200
+local N_ITERATIONS_2 = 200
-for i = 1, N_ITERATIONS do
+for i = 1, N_ITERATIONS_2 do
t[i], t[i + 1], t[i + 2] = pcall, type, {}
- coroutine.wrap(f)()
+ coroutine.wrap(pcall_wrapper)()
end
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with unpacked pcalls')
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+test:ok(true, 'no stack overflow with metamethod __call with xpcall()')
+
+-- The testcase demonstrates a stack overflow in
+-- `pcall()`/`xpcall()` similar to the first testcase, but it is
+-- triggered using `unpack()`.
+
+t = {}
+local function f()
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">--
2.43.0
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>