Hi, Sergey,

thanks for review! See comments below. The branch was force-pushed.

Sergey

Hi, Sergey!
Thanks for the patch!

LGTM, after fixing my nits below.
Please add the iterational diff for the fixes.

On 12.03.26, Sergey Bronnikov wrote:

From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry picked from commit d1a2fef8a8f53b0055ee041f7f63d83a27444ffa)

Stack overflow can cause a segmentation fault in a vararg
function on ARM64 and MIPS64 in LJ_FR2 mode. This happens
because the stack check in BC_IFUNCV is off by one on these
platforms without the patch. The original stack check
for ARM64 and MIPS64 was incorrect:

| RA == BASE + (RD=NARGS)*8 + framesize * 8 >= maxstack

while the stack check on x86_64 is correct and therefore is
not affected by the problem:

| RA == BASE + (RD=NARGS+1)*8 + framesize * 8 +8 > maxstack

Typo: s/ +8/ + 8/

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.

A fixup for a number of required slots in `call_init()` was added
for consistency with non-GC64 flavor. The check is too strict, so
this can't lead to any crash.

This patch also corrects the number of redzone slots in
luajit-gdb.py to match the updated LJ_STACK_EXTRA and adds the test

luajit_lldb.py should be updated as well.

Right, fixed:

--- a/src/luajit_lldb.py
+++ b/src/luajit_lldb.py
@@ -833,7 +833,7 @@ def dump_stack(L, base=None, top=None):
     top = top or L.top
     stack = mref(TValuePtr, L.stack)
     maxstack = mref(TValuePtr, L.maxstack)
-    red = 5 + 2 * LJ_FR2
+    red = 5 + 3 * LJ_FR2
 
     dump = [
         '{padding} Red zone: {nredslots: >2} slots {padding}'.format(

<gh-1402-call_init-regression.test.lua> that will help to avoid

gh- prefix is for the Tarantool issue tracker, use lj- for LuaJIT issue
tracker.

Ah, right, I've overlooked it is a LuaJIT issue, not Tarantool. Thanks!

Renamed.

a regression in the future, see details in [1].

Just mention details here 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.
|
| A fixup for a number of required slots in `call_init()` was added for
| consistency with the non-GC64 flavor. The check is too strict (if
| comparing the corresponding checks in the VM BC_IFUNCV), so this can't
| lead to any crash. To avoid possible regression in the future the
| corresponding test is added.
|
| This patch also corrects the number of redzone slots in luajit-gdb.py
| and luajit_lldb.py to match the updated LJ_STACK_EXTRA.

Sergey Bronnikov:
* added the description and the test for the problem

Part of tarantool/tarantool#12134

1. https://github.com/LuaJIT/LuaJIT/issues/1402

Please, don't mention the issue during backporting, to avoid messing the
issue tracker.

---
 src/lj_def.h                                  |  2 +-
 src/lj_dispatch.c                             |  2 +-
 src/luajit-gdb.py                             |  2 +-
 src/vm_arm64.dasc                             |  1 +
 src/vm_mips64.dasc                            |  1 +
 .../gh-1402-call_init-regression.test.lua     | 36 +++++++++++++

gh- prefix is for the Tarantool issue tracker, use lj- for LuaJIT issue
tracker.

 ...048-fix-stack-checks-vararg-calls.test.lua | 53 +++++++++++++++++++
 7 files changed, 94 insertions(+), 3 deletions(-)
 create mode 100644 test/tarantool-tests/gh-1402-call_init-regression.test.lua
 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

<snipped>

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

<snipped>

diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index 0ae2a6e0..dab07b35 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py

<snipped>

diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
index 6600e226..5ef37243 100644
--- a/src/vm_arm64.dasc
+++ b/src/vm_arm64.dasc

<snipped>

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

<snipped>

diff --git a/test/tarantool-tests/gh-1402-call_init-regression.test.lua b/test/tarantool-tests/gh-1402-call_init-regression.test.lua

Please, avoid _ in the file names, lets name it like:

lj-1402-vararg-stkov-check-gc64.test.lua

Same for the name of the test.

new file mode 100644
index 00000000..b20f9e39
--- /dev/null
+++ b/test/tarantool-tests/gh-1402-call_init-regression.test.lua
@@ -0,0 +1,36 @@
+local tap = require('tap')
+
+-- A test file to demonstrate a probably quite strict stack
+-- check for vararg functions in call_init.

This is not about quite strict stack check. We need this to test the
behaviour of the LuaJIT while recording the vararg function. Let's
rephrase like the following:

| -- The test file to verify correctness of stack size check during
| -- recording of vararg functions.

The test file to verify correctness of stack size check during recording of vararg functions.

+-- See also https://github.com/LuaJIT/LuaJIT/issues/1402
+local test = tap.test('gh-1402-call_init-regression.test.lua'):skipcond({

gh- prefix is for the Tarantool issue tracker, use lj- for LuaJIT issue
tracker.

renamed

--- a/test/tarantool-tests/lj-1402-vararg-stkov-check-gc64.test.lua
+++ b/test/tarantool-tests/lj-1402-vararg-stkov-check-gc64.test.lua
@@ -1,15 +1,16 @@
 local tap = require('tap')
 
--- A test file to demonstrate a probably quite strict stack
--- check for vararg functions in call_init.
+-- The test file to verify correctness of stack size check during
+-- recording of vararg functions.
 -- See also https://github.com/LuaJIT/LuaJIT/issues/1402
-local test = tap.test('gh-1402-call_init-regression.test.lua'):skipcond({
+local test = tap.test('lj-1402-vararg-stkov-check-gc64.test.lua'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
 

+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+local function vararg(...) -- luacheck: no unused

Let's use this comment before the vararg declaration.
It helps with the _ below as well.

Updated:

--- a/test/tarantool-tests/lj-1402-vararg-stkov-check-gc64.test.lua
+++ b/test/tarantool-tests/lj-1402-vararg-stkov-check-gc64.test.lua
@@ -1,15 +1,16 @@
 local tap = require('tap')
 
--- A test file to demonstrate a probably quite strict stack
--- check for vararg functions in call_init.
+-- The test file to verify correctness of stack size check during
+-- recording of vararg functions.
 -- See also https://github.com/LuaJIT/LuaJIT/issues/1402
-local test = tap.test('gh-1402-call_init-regression.test.lua'):skipcond({
+local test = tap.test('lj-1402-vararg-stkov-check-gc64.test.lua'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
 
 test:plan(1)
 
-local function vararg(...) -- luacheck: no unused
+-- luacheck: no unused
+local function vararg(...)
   -- None.
 end
 

+  -- None.
+end
+
+-- Make compilation aggressive.

Excess comment. It's quite general approach in our tests.

Updated:

 test:plan(1)
 
-local function vararg(...) -- luacheck: no unused
+-- luacheck: no unused
+local function vararg(...)
   -- None.
 end
 

+jit.opt.start("hotloop=1")

Typo: s/"/'/g

+

Please add the following comment:

| -- This function utilizes the exact amount of stack slots
| -- to cause the stack reallocation during `call_init()` in the
| -- GC64 mode.

+local function caller()
+  -- luacheck: push no unused

Lets drop this luacheck suppression, see the comment above.

Updated:

+-- This function utilizes the exact amount of stack slots to cause
+-- the stack reallocation during `call_init()` in the GC64 mode.
 local function caller()
-  -- luacheck: push no unused
   local _, _, _, _, _, _, _, _, _, _
   local _, _, _, _, _, _, _, _, _, _
   local _, _, _, _, _, _, _, _, _, _
-  -- luacheck: pop
   local n = 1
   while n < 3 do
     vararg()

+  local _, _, _, _, _, _, _, _, _, _
+  local _, _, _, _, _, _, _, _, _, _
+  local _, _, _, _, _, _, _, _, _, _
+  -- luacheck: pop
+  local n = 1
+  while n < 3 do
+    vararg()
+    n = n + 1
+  end
+end
+
+pcall(coroutine.wrap(caller))

The pcall is excess lets do it without it:
| coroutine.wrap(caller)()

+
+test:ok(true, 'no assertion for vararg functions in call_init')

Just mention 'no assertion failure' (this assertion isn't in the
`call_init()`, but during recording in `rec_check_slots()`).

Updated:

 
-test:ok(true, 'no assertion for vararg functions in call_init')
+test:ok(true, 'no assertion')
 
 test:done(true)

+
+test:done(true)
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..3a8ad63d
--- /dev/null
+++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua

<snipped>

-- 
2.43.0