Hi, Sergey, thanks for review! See comments below. The branch was force-pushed. Sergey On 3/12/26 12:36, Sergey Kaplun via Tarantool-patches wrote: > 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 >> >> 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/ Fixed, thanks! >> 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( > >> 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. > Updated. >> 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. renamed > >> ...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 > > >> 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 > > >> 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 > > >> 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 > > >> 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 > > >> 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. ok, renamed once again > >> 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. Updated. > > The test file to verify correctness of stack size check during recording of vararg functions. >> +-- See alsohttps://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 > >> +  end --- Make compilation aggressive. -jit.opt.start("hotloop=1") +jit.opt.start('hotloop=1')  local function caller()    -- luacheck: push no unused > 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. --- Make compilation aggressive. -jit.opt.start("hotloop=1") +jit.opt.start('hotloop=1') +-- 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 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)() > @@ -29,7 +29,7 @@ local function caller()    end  end -pcall(coroutine.wrap(caller)) +coroutine.wrap(caller)() test:ok(true, 'no assertion for vararg functions in call_init') >> + >> +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 > > >> -- >> 2.43.0 >>