<!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! See my comments below.</p>
<p>Sergey</p>
<div class="moz-cite-prefix">On 3/12/26 13:16, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:abKSkAwu0v9QGt35@root">
<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>
Updated
<blockquote type="cite" cite="mid:abKSkAwu0v9QGt35@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/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(-)
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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
index 5ef37243..074c1f31 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_mips.dasc b/src/vm_mips.dasc
index 32caabf7..69d09d52 100644
--- a/src/vm_mips.dasc
+++ b/src/vm_mips.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 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)
| 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
@@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)
|. nop
|
|.ffunc xpcall
- | 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
</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)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ | 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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
This line is incorrect. This neglets the 1st patch in the series.
It should be
| | 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
</pre>
</blockquote>
<p>Right. However, probably we should leave this line near ".ffunc
xpcall". What do you think?</p>
<p>Now updated as the following:</p>
<p>--- a/src/vm_mips64.dasc<br>
+++ b/src/vm_mips64.dasc<br>
@@ -1449,7 +1449,7 @@ static void build_subroutines(BuildCtx *ctx)<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:RC">NARGS8:RC</a>, <a class="moz-txt-link-freetext" href="NARGS8:RC">NARGS8:RC</a>, -16<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 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:abKSkAwu0v9QGt35@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> | 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)
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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
index 8b6781a6..c57b76b7 100644
--- a/src/vm_x64.dasc
+++ b/src/vm_x64.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_x86.dasc b/src/vm_x86.dasc
index 7c11c78e..36804d11 100644
--- a/src/vm_x86.dasc
+++ b/src/vm_x86.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
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 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')
-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>
<p>Updated</p>
<p>@@ -53,7 +53,8 @@ <a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with
metamethod')<br>
-- The testcases demonstrates a stack overflow in<br>
-- `pcall()`/xpcall()` triggered using metamethod `__call`.<br>
<br>
-t = coroutine.wrap(setmetatable)({}, { __call = pcall })<br>
+t = setmetatable({}, { __call = pcall })<br>
+coroutine.wrap(function() t() end)()<br>
<br>
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with metamethod __call with
pcall()')<br>
<br>
<br>
</p>
<blockquote type="cite" cite="mid:abKSkAwu0v9QGt35@root">
<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>
<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 6395dfaa..825568f9 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>
@@ -58,7 +58,17 @@ coroutine.wrap(function() t() end)()<br>
<br>
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with metamethod __call with
pcall()')<br>
<br>
-t = coroutine.wrap(setmetatable)({}, { __call = xpcall })<br>
+t = {}<br>
+local function xpcall_wrapper()<br>
+ return xpcall(unpack(t))<br>
+end<br>
+<br>
+local N_ITERATIONS_1 = 200<br>
+<br>
+for i = 1, N_ITERATIONS_1 do<br>
+ t[i], t[i + 1], t[i + 2] = xpcall, type, {}<br>
+ coroutine.wrap(xpcall_wrapper)()<br>
+end<br>
<br>
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with metamethod __call with
xpcall()')<br>
<br>
@@ -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()')<br>
-- triggered using `unpack()`.<br>
<br>
t = {}<br>
-local function f()<br>
+local function pcall_wrapper()<br>
return pcall(unpack(t))<br>
end<br>
<br>
--- The problem is only reproduced on LuaJIT GC64 and is best<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 = 200<br>
+local N_ITERATIONS_2 = 200<br>
<br>
-for i = 1, N_ITERATIONS do<br>
+for i = 1, N_ITERATIONS_2 do<br>
t[i], t[i + 1], t[i + 2] = pcall, type, {}<br>
- coroutine.wrap(f)()<br>
+ coroutine.wrap(pcall_wrapper)()<br>
end<br>
<br>
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with unpacked pcalls')<br>
<br>
</p>
<blockquote type="cite" cite="mid:abKSkAwu0v9QGt35@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<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>
<pre wrap="" class="moz-quote-pre">
Lets name it `pcall_wrapper()`
</pre>
</blockquote>
@@ -66,7 +68,7 @@ <a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with metamethod
__call with xpcall()')<br>
-- triggered using `unpack()`.<br>
<br>
t = {}<br>
-local function f()<br>
+local function pcall_wrapper()<br>
return pcall(unpack(t))<br>
end<br>
<br>
@@ -78,7 +80,7 @@ local N_ITERATIONS = 200<br>
<br>
for i = 1, N_ITERATIONS do<br>
t[i], t[i + 1], t[i + 2] = pcall, type, {}<br>
- coroutine.wrap(f)()<br>
+ coroutine.wrap(pcall_wrapper)()<br>
end<br>
<br>
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>, 'no stack overflow with unpacked pcalls')<br>
<br>
<blockquote type="cite" cite="mid:abKSkAwu0v9QGt35@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ return pcall(unpack(t))
+end
+
+-- The problem is only reproduced on LuaJIT GC64 and is best
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/best/better/</pre>
</blockquote>
return pcall(unpack(t))<br>
end<br>
<br>
--- The problem is only reproduced on LuaJIT GC64 and is best<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>
<br>
<blockquote type="cite" cite="mid:abKSkAwu0v9QGt35@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- 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
+
+for i = 1, N_ITERATIONS do
+ t[i], t[i + 1], t[i + 2] = pcall, type, {}
+ 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>