<!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 consider my three comments below.</p>
<p>Sergey</p>
<div class="moz-cite-prefix">On 10/27/25 11:16, Sergey Kaplun wrote:<br>
</div>
<blockquote type="cite" cite="mid:aP8qWSAbrPi3gHTf@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the fixes!
Please consider my comments below.
Also, please send the next version via v2 series to simplify the
review.
On 23.09.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Hi, Sergey,
thanks for review! Please see my comments below.
Sergey
On 9/1/25 16:07, Sergey Kaplun via Tarantool-patches wrote:
</pre>
<blockquote type="cite">
<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>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<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#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>
</blockquote>
</blockquote>
<p>The LJ_FR2 check was added for consistency with non-gc64 flavor,
the commit message was updated</p>
<p>> A fixup for a number of required slots in `call_init()`
was added<br>
> for consistency with non-gc64 flavor.<br>
</p>
<p>Also, there is an issue [1] about inconsistencies in stack
checks.</p>
<p>1. <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1402">https://github.com/LuaJIT/LuaJIT/issues/1402</a></p>
<blockquote type="cite" cite="mid:aP8qWSAbrPi3gHTf@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
Please check again. Both testcases trigger segfault on AArch64 (odroid).
</pre>
</blockquote>
</blockquote>
<br>
<blockquote type="cite" cite="mid:aP8qWSAbrPi3gHTf@root">
<pre wrap="" class="moz-quote-pre">
Double checked:
| root@<a class="moz-txt-link-freetext" href="odroid:/home/skaplun/lj-1048-review#">odroid:/home/skaplun/lj-1048-review#</a> git diff
| diff --git a/src/lj_dispatch.c b/src/lj_dispatch.c
| index 431cb3c2..a44a5adf 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+LJ_FR2+gotparams;
| + if ((pt->flags & PROTO_VARARG)) need += 1+gotparams;
| lj_state_checkstack(L, (MSize)need);
| numparams -= gotparams;
| return numparams >= 0 ? numparams : 0;
| Test project /home/skaplun/lj-1048-review
| Start 118: test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
| 1/1 Test #118: test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua ... Passed 3.38 sec
|
| 100% tests passed, 0 tests failed out of 1
|
| Label Time Summary:
| tarantool-tests = 3.38 sec*proc (1 test)
|
| Total Test time (real) = 3.42 sec
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<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?
</pre>
</blockquote>
</blockquote>
</blockquote>
`prober_1` triggers the issue by using recursive (p)call
<blockquote type="cite" cite="mid:aP8qWSAbrPi3gHTf@root">
<pre wrap="" class="moz-quote-pre">
Still need an explanation.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
If we want to keep it, please describe why we need at least 9 pcall-s.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">As I got right, exactly this number of pcall's is needed to trigger a
stack overflow.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Yes, but why 9 is minimum number of pcall's when the issue is reproduced?
<snipped></pre>
</blockquote>
<p>The number depends on a previous value of LJ_STACK_EXTRA.</p>
<div class="lRu31" dir="ltr"><span class="HwtZe" lang="en"><span
class="jCAhz ChMk0b"><span class="ryNqvb">LJ_STACK_EXTRA, is
an "overlay" on top of the stack, and for a buffer overflow</span></span></span></div>
<div class="lRu31" dir="ltr"><span class="HwtZe" lang="en"><span
class="jCAhz ChMk0b"><span class="ryNqvb">you need at least 8
+ 1 frames to write slots above this "overlay".</span></span></span><span
class="ZSCsVd"> These pcalls generates additional frames.</span>
<div class="OvtS8d"><span style="white-space: pre-wrap">
</span></div>
</div>
<blockquote type="cite" cite="mid:aP8qWSAbrPi3gHTf@root">
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>