Hi, Sergey, thanks for review! On 2/11/26 11:30, Sergey Kaplun via Tarantool-patches wrote: > On 10.12.25, Sergey Bronnikov wrote: >> From: Mike Pall >> >> Thanks to Peter Cawley. >> >> (cherry picked from commit d1a2fef8a8f53b0055ee041f7f63d83a27444ffa) >> >> Stack overflow can cause a segmentation fault in vararg > Typo: s/vararg/a vararg/ Fixed. > >> function on ARM64 and MIPS64 in LJ_FR2 mode. This happen > Typo: s/happen/happens/ Fixed. > >> because stack check in BC_IFUNCV is off by one on these > Typo: s/stack/the stack/ Fixed. > >> platforms without the patch. The original stack check >> for ARM64 and MIPS64 was incorrect: >> >> | RA == BASE + (RD=NARGS)*8 + framesize * 8 >= maxstack >> >> while stack check on x86_64 is correct and therefore is > Typo: s/stack/the stack/ Fixed. > >> not affected by the problem: >> >> | RA == BASE + (RD=NARGS+1)*8 + framesize * 8 +8 > maxstack >> >> The patch partially fixes aforementioned issue by > Typo: s/aforementioned/the aforementioned/ Fixed. > >> bumping LJ_STACK_EXTRA by 1 to give a space to write >> the entire frame link and fixing a number of last > Typo: s/a number/the number/ Fixed. > > I'm not get this part. I suggest rephrasing it 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. Updated. > > >> free slot in the stack (LJ_FR2 summand adjustment). >> >> A fixup for a number of required slots in `call_init()` was added >> for consistency with non-gc64 flavor. > Typo: s/gc64/GC64/ Fixed. > I would also add: "The check is too strict, so this can't lead to any > crash." Updated. >> Sergey Bronnikov: >> * added the description and the test for the problem >> >> Part of tarantool/tarantool#12134 >> --- >> 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 | 53 +++++++++++++++++++ >> 5 files changed, 57 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 >> index a5bca6b0..7e4f251e 100644 >> --- a/src/lj_def.h >> +++ b/src/lj_def.h >> @@ -69,7 +69,7 @@ typedef unsigned int uintptr_t; >> #define LJ_MAX_UPVAL 60 /* Max. # of upvalues. */ >> >> #define LJ_MAX_IDXCHAIN 100 /* __index/__newindex chain limit. */ >> -#define LJ_STACK_EXTRA (5+2*LJ_FR2) /* Extra stack space (metamethods). */ >> +#define LJ_STACK_EXTRA (5+3*LJ_FR2) /* Extra stack space (metamethods). */ >> >> #define LJ_NUM_CBPAGE 1 /* Number of FFI callback pages. */ >> >> 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; >> lj_state_checkstack(L, (MSize)need); >> numparams -= gotparams; >> return numparams >= 0 ? numparams : 0; > Let's add an additional test for this part of code (since we don't have > any). It may be taken from [1]. It doesn't fail now, but we may cover > this branch more precise. Don't get what do you mean. true branch in gc32 is covered by the following tests: test/LuaJIT-tests test/PUC-Rio-Lua-5.1-tests test/tarantool-c-tests/lj-1087-vm-handler-call.c_test test/tarantool-tests/fix-ff-select-recording.test.lua test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua test/tarantool-tests/fix-slot-check-for-mm-record.test.lua test/tarantool-tests/fix-slots-overflow-for-varg-record.test.lua test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua test/tarantool-tests/lj-1024-varg-maxslot.test.lua test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua test/tarantool-tests/lj-1164-record-meta-concat-varg-pcall.test.lua test/tarantool-tests/lj-1295-bad-renames-for-sunk-values.test.lua test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua test/tarantool-tests/lj-704-bc-varg-use-def.test.lua >> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc >> index c5f0a7a7..cf8e575a 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/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..d471d41e >> --- /dev/null >> +++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua >> @@ -0,0 +1,53 @@ >> +local tap = require('tap') >> + >> +-- A test file to demonstrate a stack overflow in `pcall()` in > I would rephrase it like "demonstrate a crash due to Lua stack > out-of-bounds access". Updated: --- 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,7 +1,7 @@  local tap = require('tap') --- A test file to demonstrate a stack overflow in `pcall()` in --- some cases, see below testcase descriptions. +-- A test file to demonstrate a crash due to Lua stack +-- out-of-bounds access, see below testcase descriptions.  -- See also https://github.com/LuaJIT/LuaJIT/issues/1048.  local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({    ['Test requires JIT enabled'] = not jit.status(), > >> +-- some cases, see below testcase descriptions. >> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/1048. >> +local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({ >> + ['Test requires JIT enabled'] = not jit.status(), >> +}) > I suppose there is no need in JIT here. This skipcond may be removed. Removed. >> + >> +test:plan(2) >> + >> +-- The testcase demonstrate a segmentation fault due to stack > Typo: s/testcase demonstrate/test case demonstrates/ Updated. > >> +-- overflow by recursive calling `pcall()`. The functions are >> +-- vararg because stack check in BC_IFUNCV is off by one on ARM64 > Typo: s/stack/the stack/ Updated. > >> +-- and MIPS64 without the patch. >> +local function prober_1(...) -- luacheck: no unused >> + -- Any fast function can be used as metamethod, but `type` is >> + -- convenient here because it works fast and can be used with >> + -- any data type. Lua function cannot be used since it >> + -- will check the stack on each invocation. > Please add the comment, that we need to check using of the correct > value LJ_STACK_EXTRA slots (5+3*LJ_FR2) = 8 for GC64 mode. Updated:  local function prober_1(...) -- luacheck: no unused    -- Any fast function can be used as metamethod, but `type` is    -- convenient here because it works fast and can be used with    -- any data type. Lua function cannot be used since it -  -- will check the stack on each invocation. +  -- will check the stack on each invocation. We need to check +  -- using of the correct value LJ_STACK_EXTRA slots +  -- (5+3*LJ_FR2) = 8 for GC64 mode.    pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, type, 0)  end > >> + pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, type, 0) >> +end >> + >> +local function looper(prober, n, ...) >> + prober(...) >> + return looper(prober, n + 1, n, ...) >> +end >> + >> +pcall(coroutine.wrap(looper), prober_1, 0) >> + >> +test:ok(true, 'no stack overflow with recursive pcall') >> + >> +-- The testcase demonstrate a segmentation fault due to stack > Typo: s/testcase demonstrate/test case demonstrates/ Updated. > >> +-- overflow when `pcall()` is used as `__newindex` metamethod. >> +-- The function is vararg because stack check in BC_IFUNCV is off > Typo: s/stack/the stack/ Updated. > >> +-- by one on ARM64 and MIPS64 without the patch. >> + >> +-- Any fast function can be used as metamethod, but `type` is > Typo: s/metamethod/a metamethod/ Fixed. > >> +-- convenient here because it works fast and can be used with >> +-- any data type. Lua function cannot be used since it > Typo: s/Lua/The Lua/ Fixed. > >> +-- will check the stack on each invocation. >> +local t = setmetatable({}, { __newindex = pcall, __call = type }) >> + >> +local function prober_2(...) -- luacheck: no unused >> + -- Invokes `pcall(t, t, t)`. >> + t[t] = t >> +end >> + >> +pcall(coroutine.wrap(looper), prober_2, 0) >> + >> +test:ok(true, 'no stack overflow with metamethod') >> + >> +test:done(true) >> -- >> 2.43.0 >> > [1]:https://github.com/LuaJIT/LuaJIT/issues/1402#issue-3569942423 >