Re: [Tarantool-patches] [PATCH luajit 3/3][v3] Add stack check to pcall/xpcall.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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 <mike>
> >>
> >> 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(-)
> >>

<snipped>

> >> 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).

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)
> -  |  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)

<snipped>

> >> 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

Why do we need two variables with the same value of iterations?
Let's use N_ITERATIONS with the comment for xpcall and pcall.

> +
> +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()

<snipped>

> >> -- 
> >> 2.43.0
> >>

-- 
Best regards,
Sergey Kaplun