<!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>
<p>Sergey</p>
<div class="moz-cite-prefix">On 2/11/26 13:24, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:aYxY2jP8eBh6Z6MF@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On 10.12.25, 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)
In the previous commit ("LJ_FR2: Fix stack checks in vararg calls.")
stack overflow for vararg functions and metamethod invocations
was fixed partially and there are still cases where stack overflow
happens, see comments in the test. The patch fixes the issue by
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">Please describe the issue regardless previous commit. Just mentioned
missing stack checks for `pcall()`, `xpcall()` is enough.
</pre>
</blockquote>
<p>Updated commit message:</p>
<p><br>
Add stack check to pcall/xpcall.<br>
<br>
Analyzed by Peter Cawley.<br>
<br>
(cherry picked from commit
a4c1640432a9d8a60624cdc8065b15078c228e36)<br>
<br>
The patch adds the stack check to fast functions `pcall()` and<br>
`xpcall()`.<br>
<br>
Sergey Bronnikov:<br>
* added the description and the test for the problem<br>
<br>
Part of tarantool/tarantool#12134<br>
<br>
<br>
</p>
<blockquote type="cite" cite="mid:aYxY2jP8eBh6Z6MF@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">adding the stack check to fast functions `pcall()` and `xpcall()`.
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 | 31 ++++++++++++++++++-
8 files changed, 86 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 cf8e575a..53ff7162 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 change is incorrect.
It wipes out the first patch in the series.
</pre>
</blockquote>
<p>Sorry, my bad.</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:aYxY2jP8eBh6Z6MF@root">
<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)
+ | 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 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 d5296759..141f5f82 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 b043b830..1ba5abce 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 d471d41e..b135042b 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
@@ -1,4 +1,5 @@
local tap = require('tap')
+local ffi = require('ffi')
-- A test file to demonstrate a stack overflow in `pcall()` in
-- some cases, see below testcase descriptions.
@@ -7,7 +8,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)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">This patch covers only `pcall()` cases, please add the same tests for
`xpcall()` (I suppose the most simple is `xpcall()` as `__call`
metamethod).</pre>
</blockquote>
<p>Added:</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 0ce3c756..8f45941c 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>
@@ -6,7 +6,7 @@ local ffi = require('ffi')<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')<br>
<br>
-test:plan(4)<br>
+test:plan(5)<br>
<br>
-- The test case demonstrates a segmentation fault due to stack<br>
-- overflow by recursive calling `pcall()`. The functions are<br>
@@ -51,12 +51,16 @@ pcall(coroutine.wrap(looper), prober_2, 0)<br>
<br>
<a class="moz-txt-link-freetext" href="test:ok(true">test:ok(true</a>,
'no stack overflow with metamethod')<br>
<br>
--- The testcase demonstrates a stack overflow in<br>
+-- The testcases demonstrates a stack overflow in<br>
-- `pcall()`/xpcall()` triggered using metamethod `__call`.<br>
<br>
t = coroutine.wrap(setmetatable)({}, { __call = pcall })<br>
<br>
-test:ok(true, 'no stack overflow with metamethod __call')<br>
+test:ok(true, 'no stack overflow with metamethod __call with
pcall()')<br>
+<br>
+t = coroutine.wrap(setmetatable)({}, { __call = xpcall })<br>
+<br>
+test:ok(true, 'no stack overflow with metamethod __call with
xpcall()')<br>
<br>
-- The testcase demonstrates a stack overflow in<br>
-- `pcall()`/`xpcall()` similar to the first testcase, but it is<br>
<br>
</p>
<blockquote type="cite" cite="mid:aYxY2jP8eBh6Z6MF@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
-- The testcase demonstrate a segmentation fault due to stack
-- overflow by recursive calling `pcall()`. The functions are
@@ -50,4 +51,32 @@ 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 testcase demonstrate a stack overflow in
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">Typo: s/demonstrate/demonstrates/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYxY2jP8eBh6Z6MF@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- `pcall()`/xpcall()` triggered using metamethod `__call`.
+
+t = setmetatable({}, { __call = pcall })()
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">It's better to do this at the separate Lua stack, i.e. inside
`coroutine.wrap()`.</pre>
</blockquote>
<p>Updated.</p>
<p> -- `pcall()`/xpcall()` triggered using metamethod `__call`.<br>
<br>
-t = setmetatable({}, { __call = pcall })()<br>
+t = coroutine.wrap(setmetatable)({}, { __call = pcall })</p>
<blockquote type="cite" cite="mid:aYxY2jP8eBh6Z6MF@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+test:ok(true, 'no stack overflow with metamethod __call')
+
+-- The testcase demonstrate a stack overflow in
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">Typo: s/demonstrate/demonstrates/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aYxY2jP8eBh6Z6MF@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- `pcall()`/`xpcall()` similar to the first testcase, but it is
+-- triggered using `unpack()`.
+
+t = {}
+local function f()
+ return pcall(unpack(t))
+end
+
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">Please explain the amount of necessary iterations of calls.</pre>
</blockquote>
<p>After a small research we found that issue cannot be reproduced
in LuaJIT GC32 flavors.</p>
<p>N_ITERATION should be equal to at least 200 for reproducing.</p>
<p>The problem reproduced better under Valgrind than ASAN.</p>
<p><span style="white-space: pre-wrap">
</span><br>
</p>
<blockquote type="cite" cite="mid:aYxY2jP8eBh6Z6MF@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+local N_ITERATIONS = 100
+if ffi.abi('gc64') then
+ N_ITERATIONS = 180
+end
+
+for i = 1, N_ITERATIONS do
+ t[i], t[i + 1], t[i + 2] = pcall, pairs, {}
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">Let's use `type` here instead of pairs.</pre>
</blockquote>
Updated.
<blockquote type="cite" cite="mid:aYxY2jP8eBh6Z6MF@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ 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>
</blockquote>
<lt-container></lt-container>
</body>
</html>