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

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

<snipped>

> 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

<snipped>

> 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

<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)
>    |  daddiu NARGS8:RC, NARGS8:RC, -8
> -  |  lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
>    |  bltz NARGS8:RC, ->fff_fallback
>    |.   move TMP2, BASE
>    |   daddiu BASE, BASE, 16
> @@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)
>    |.  nop
>    |
>    |.ffunc xpcall
> -  |  daddiu NARGS8: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)

> +  |  daddiu NARGS8:RC, NARGS8:RC, -16

This line is incorrect. This neglets the 1st patch in the series.

It should be
| |  daddiu NARGS8:TMP0, NARGS8:RC, -16


>    |   ld CARG2, 8(BASE)
>    |    bltz NARGS8: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

<snipped>

> 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

<snipped>

> 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

<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 also https://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)()

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

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

> +  return pcall(unpack(t))
> +end
> +
> +-- The problem is only reproduced on LuaJIT GC64 and is best

Typo: s/best/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 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
> 

-- 
Best regards,
Sergey Kaplun