Hi, Sergey, thanks for review! See my comments below. Sergey On 3/12/26 13:16, Sergey Kaplun via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the patch! > Please, fix my comments below. > > Don't forget to add the corresponding iterative changes. > > On 12.03.26, Sergey Bronnikov wrote: >> From: Mike Pall >> >> Analyzed by Peter Cawley. >> >> (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36) >> >> The patch adds the stack check to fast functions `pcall()` and >> `xpcall()`. > Please add more verbose description: > > | (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36) > | > | The `pcall()` and `xpcall()` calls in GC64 mode require 2 slots. This > | means that all arguments should be moved up during emitting of the frame > | link to the stack. Hence, this may cause stack overflow without the > | corresponding check. > | > | This patch adds the corresponding checks to the VM. Non-GC64 VMs are > | updated as well for the consistency. Updated >> 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 | 35 ++++++++++++++++++- >> 8 files changed, 90 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 5ef37243..074c1f31 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 neglets the first patch in the series. See the comment below. > >> - | 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 > This line is incorrect. This neglets the 1st patch in the series. > > It should be > | | daddiuNARGS8:TMP0,NARGS8:RC, -16 Right. However, probably we should leave this line near ".ffunc xpcall". What do you think? Now updated as the following: --- 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 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 8b6781a6..c57b76b7 100644 >> --- a/src/vm_x64.dasc >> +++ b/src/vm_x64.dasc > > >> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc >> index 7c11c78e..36804d11 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 3a8ad63d..ad8b151b 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 >> @@ -5,7 +5,7 @@ local tap = require('tap') >> -- See alsohttps://github.com/LuaJIT/LuaJIT/issues/1048. >> local test = tap.test('lj-1048-fix-stack-checks-vararg-calls') >> >> -test:plan(2) >> +test:plan(5) >> >> -- The test case demonstrates a segmentation fault due to stack >> -- overflow by recursive calling `pcall()`. The functions are >> @@ -50,4 +50,37 @@ pcall(coroutine.wrap(looper), prober_2, 0) >> >> test:ok(true, 'no stack overflow with metamethod') >> >> +-- The testcases demonstrates a stack overflow in >> +-- `pcall()`/xpcall()` triggered using metamethod `__call`. >> + >> +t = coroutine.wrap(setmetatable)({}, { __call = pcall }) > I've meant the following: > > | t = setmetatable({}, { __call = pcall }) > | coroutine.wrap(function() t() end)() > Updated @@ -53,7 +53,8 @@ test:ok(true, 'no stack overflow with metamethod')  -- The testcases demonstrates a stack overflow in  -- `pcall()`/xpcall()` triggered using metamethod `__call`. -t = coroutine.wrap(setmetatable)({}, { __call = pcall }) +t = setmetatable({}, { __call = pcall }) +coroutine.wrap(function() t() end)() test:ok(true, 'no stack overflow with metamethod __call with pcall()') >> + >> +test:ok(true, 'no stack overflow with metamethod __call with pcall()') >> + >> +t = coroutine.wrap(setmetatable)({}, { __call = xpcall }) > I've meant the following: > > | t = setmetatable({}, { __call = xpcall }) > | coroutine.wrap(function() t() end)() > > But this won't work since the second amount of xpcall must be the > function. So, this test case is invalid. We must to duplicate the second > approach with `xpcall()` > > This works fine. > | LUA_PATH="src/?.lua;;" gdb --args src/luajit -e ' > | local t = {} > | local function xpcall_wrapper() > | return xpcall(unpack(t)) > | end > | > | local N_ITERATIONS = 200 > | > | for i = 1, N_ITERATIONS do > | t[i], t[i + 1], t[i + 2] = xpcall, type, {} > | coroutine.wrap(xpcall_wrapper)() > | end > | ' Updated: 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 6395dfaa..825568f9 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 @@ -58,7 +58,17 @@ coroutine.wrap(function() t() end)() test:ok(true, 'no stack overflow with metamethod __call with pcall()') -t = coroutine.wrap(setmetatable)({}, { __call = xpcall }) +t = {} +local function xpcall_wrapper() +  return xpcall(unpack(t)) +end + +local N_ITERATIONS_1 = 200 + +for i = 1, N_ITERATIONS_1 do +  t[i], t[i + 1], t[i + 2] = xpcall, type, {} +  coroutine.wrap(xpcall_wrapper)() +end test:ok(true, 'no stack overflow with metamethod __call with xpcall()') @@ -67,19 +77,19 @@ test:ok(true, 'no stack overflow with metamethod __call with xpcall()')  -- triggered using `unpack()`.  t = {} -local function f() +local function pcall_wrapper()    return pcall(unpack(t))  end --- The problem is only reproduced on LuaJIT GC64 and is best +-- The problem is only reproduced on LuaJIT GC64 and is better  -- reproduced under Valgrind than AddressSanitizer. The chosen  -- value was found experimentally and always results in an attempt  -- to write beyond the allocated memory. -local N_ITERATIONS = 200 +local N_ITERATIONS_2 = 200 -for i = 1, N_ITERATIONS do +for i = 1, N_ITERATIONS_2 do    t[i], t[i + 1], t[i + 2] = pcall, type, {} -  coroutine.wrap(f)() +  coroutine.wrap(pcall_wrapper)()  end test:ok(true, 'no stack overflow with unpacked pcalls') >> + >> +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 >> +-- triggered using `unpack()`. >> + >> +t = {} >> +local function f() > Lets name it `pcall_wrapper()` > @@ -66,7 +68,7 @@ test:ok(true, 'no stack overflow with metamethod __call with xpcall()')  -- triggered using `unpack()`.  t = {} -local function f() +local function pcall_wrapper()    return pcall(unpack(t))  end @@ -78,7 +80,7 @@ local N_ITERATIONS = 200  for i = 1, N_ITERATIONS do    t[i], t[i + 1], t[i + 2] = pcall, type, {} -  coroutine.wrap(f)() +  coroutine.wrap(pcall_wrapper)()  end test:ok(true, 'no stack overflow with unpacked pcalls') >> + return pcall(unpack(t)) >> +end >> + >> +-- The problem is only reproduced on LuaJIT GC64 and is best > Typo: s/best/better/    return pcall(unpack(t))  end --- The problem is only reproduced on LuaJIT GC64 and is best +-- The problem is only reproduced on LuaJIT GC64 and is better  -- reproduced under Valgrind than AddressSanitizer. The chosen  -- value was found experimentally and always results in an attempt  -- to write beyond the allocated memory. > >> +-- reproduced under Valgrind than AddressSanitizer. The chosen >> +-- value was found experimentally and always results in an attempt >> +-- to write beyond the allocated memory. >> +local N_ITERATIONS = 200 >> + >> +for i = 1, N_ITERATIONS do >> + t[i], t[i + 1], t[i + 2] = pcall, type, {} >> + coroutine.wrap(f)() >> +end >> + >> +test:ok(true, 'no stack overflow with unpacked pcalls') >> + >> test:done(true) >> -- >> 2.43.0 >>