Hi, Sergey, thanks for review! Sergey On 2/11/26 13:24, Sergey Kaplun via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the patch! > Please consider my comments below. > > On 10.12.25, Sergey Bronnikov wrote: >> From: Mike Pall >> >> 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 > Please describe the issue regardless previous commit. Just mentioned > missing stack checks for `pcall()`, `xpcall()` is enough. Updated commit message:     Add stack check to pcall/xpcall.     Analyzed by Peter Cawley.     (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)     The patch adds 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 >> 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 > > >> 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 > > >> 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 > > >> 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,NARGS8:RC >> + | sltu AT, TMP1, TMP2 >> + | bnez AT, ->fff_fallback >> + |. lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH) >> | daddiuNARGS8:RC,NARGS8:RC, -8 >> - | lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH) >> | bltzNARGS8:RC, ->fff_fallback >> |. move TMP2, BASE >> | daddiu BASE, BASE, 16 >> @@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx) >> |. nop >> | >> |.ffunc xpcall >> - | daddiuNARGS8:TMP0,NARGS8:RC, -16 > This change is incorrect. > It wipes out the first patch in the series. Sorry, my bad. --- a/src/vm_mips64.dasc +++ b/src/vm_mips64.dasc @@ -1449,7 +1449,7 @@ static void build_subroutines(BuildCtx *ctx)    |  sltu AT, TMP1, TMP2    |  bnez AT, ->fff_fallback    |.  ld CARG1, 0(BASE) -  |  daddiu NARGS8:RC, NARGS8:RC, -16 +  |  daddiu NARGS8:TMP0, NARGS8:RC, -16    |   ld CARG2, 8(BASE)    |    bltz NARGS8:TMP0, ->fff_fallback    |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH) >> - | ld CARG1, 0(BASE) >> + | ld TMP1, L->maxstack >> + | daddu TMP2, BASE,NARGS8:RC >> + | sltu AT, TMP1, TMP2 >> + | bnez AT, ->fff_fallback >> + |. ld CARG1, 0(BASE) >> + | daddiuNARGS8:RC,NARGS8:RC, -16 >> | ld CARG2, 8(BASE) >> | bltzNARGS8:TMP0, ->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 > > >> 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 > > >> 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 > > >> 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) > This patch covers only `pcall()` cases, please add the same tests for > `xpcall()` (I suppose the most simple is `xpcall()` as `__call` > metamethod). Added: 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 0ce3c756..8f45941c 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 @@ -6,7 +6,7 @@ local ffi = require('ffi')  -- See also https://github.com/LuaJIT/LuaJIT/issues/1048.  local test = tap.test('lj-1048-fix-stack-checks-vararg-calls') -test:plan(4) +test:plan(5)  -- The test case demonstrates a segmentation fault due to stack  -- overflow by recursive calling `pcall()`. The functions are @@ -51,12 +51,16 @@ pcall(coroutine.wrap(looper), prober_2, 0) test:ok(true, 'no stack overflow with metamethod') --- The testcase demonstrates a stack overflow in +-- The testcases demonstrates a stack overflow in  -- `pcall()`/xpcall()` triggered using metamethod `__call`.  t = coroutine.wrap(setmetatable)({}, { __call = pcall }) -test:ok(true, 'no stack overflow with metamethod __call') +test:ok(true, 'no stack overflow with metamethod __call with pcall()') + +t = coroutine.wrap(setmetatable)({}, { __call = xpcall }) + +test:ok(true, 'no stack overflow with metamethod __call with xpcall()')  -- The testcase demonstrates a stack overflow in  -- `pcall()`/`xpcall()` similar to the first testcase, but it is >> >> -- 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) >> >> test:ok(true, 'no stack overflow with metamethod') >> >> +-- The testcase demonstrate a stack overflow in > Typo: s/demonstrate/demonstrates/ Fixed. >> +-- `pcall()`/xpcall()` triggered using metamethod `__call`. >> + >> +t = setmetatable({}, { __call = pcall })() > It's better to do this at the separate Lua stack, i.e. inside > `coroutine.wrap()`. Updated.  -- `pcall()`/xpcall()` triggered using metamethod `__call`. -t = setmetatable({}, { __call = pcall })() +t = coroutine.wrap(setmetatable)({}, { __call = pcall }) >> + >> +test:ok(true, 'no stack overflow with metamethod __call') >> + >> +-- The testcase demonstrate a stack overflow in > Typo: s/demonstrate/demonstrates/ Fixed. >> +-- `pcall()`/`xpcall()` similar to the first testcase, but it is >> +-- triggered using `unpack()`. >> + >> +t = {} >> +local function f() >> + return pcall(unpack(t)) >> +end >> + > Please explain the amount of necessary iterations of calls. After a small research we found that issue cannot be reproduced in LuaJIT GC32 flavors. N_ITERATION should be equal to at least 200 for reproducing. The problem reproduced better under Valgrind than ASAN. >> +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, {} > Let's use `type` here instead of pairs. Updated. >> + coroutine.wrap(f)() >> +end >> + >> +test:ok(true, 'no stack overflow with unpacked pcalls') >> + >> test:done(true) >> -- >> 2.43.0 >>