Hi, Sergey, I've updated N_ITERATIONS and force-pushed the branch. Sergey On 3/12/26 20:19, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the fixes! > > LGTM, after fixing the last nit below. > > On 12.03.26, Sergey Bronnikov wrote: >> 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_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? > Why? This break the `maxstack` check (since the RC is differs before the > addition with TMP2). Ok, let's leave as is, seems I took a look on intermediate version, before applying changes you requested on review. The current patch looks fine for me: @@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)    |.  nop    |    |.ffunc xpcall +  |  ld TMP1, L->maxstack +  |  daddu TMP2, BASE, NARGS8:RC +  |  sltu AT, TMP1, TMP2 +  |  bnez AT, ->fff_fallback +  |.  ld CARG1, 0(BASE)    |  daddiu NARGS8:TMP0, NARGS8:RC, -16 -  |  ld CARG1, 0(BASE)    |   ld CARG2, 8(BASE)    |    bltz NARGS8:TMP0, ->fff_fallback    |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH) > > See the latest LuaJIT version: > https://github.com/LuaJIT/LuaJIT/blob/659a61693aa3b87661864ad0f12eee14c865cd7f/src/vm_mips64.dasc#L1450 > >> 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) >> -  |  daddiuNARGS8:RC,NARGS8:RC, -16 >> +  |  daddiuNARGS8:TMP0,NARGS8:RC, -16 >>    |   ld CARG2, 8(BASE) >>    |    bltzNARGS8: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/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') >>>> -- Seealsohttps://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 > Why do we need two variables with the same value of iterations? > Let's use N_ITERATIONS with the comment for xpcall and pcall. 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 825568f9..036d53e9 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 @@ -63,9 +63,13 @@ local function xpcall_wrapper()    return xpcall(unpack(t))  end -local N_ITERATIONS_1 = 200 +-- The problems are 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 -for i = 1, N_ITERATIONS_1 do +for i = 1, N_ITERATIONS do    t[i], t[i + 1], t[i + 2] = xpcall, type, {}    coroutine.wrap(xpcall_wrapper)()  end @@ -81,13 +85,7 @@ local function pcall_wrapper()    return pcall(unpack(t))  end --- 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_2 = 200 - -for i = 1, N_ITERATIONS_2 do +for i = 1, N_ITERATIONS do    t[i], t[i + 1], t[i + 2] = pcall, type, {}    coroutine.wrap(pcall_wrapper)()  end > >> + >> +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() > > >>>> -- >>>> 2.43.0 >>>>