Re: [Tarantool-patches] [PATCH luajit 2/3][v2] LJ_FR2: Fix stack checks in vararg calls.

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

On 10.12.25, Sergey Bronnikov wrote:
> From: Mike Pall <mike>
> 
> Thanks to Peter Cawley.
> 
> (cherry picked from commit d1a2fef8a8f53b0055ee041f7f63d83a27444ffa)
> 
> Stack overflow can cause a segmentation fault in vararg

Typo: s/vararg/a vararg/

> function on ARM64 and MIPS64 in LJ_FR2 mode. This happen

Typo: s/happen/happens/

> because stack check in BC_IFUNCV is off by one on these

Typo: s/stack/the stack/

> platforms without the patch. The original stack check
> for ARM64 and MIPS64 was incorrect:
> 
> | RA == BASE + (RD=NARGS)*8 + framesize * 8 >= maxstack
> 
> while stack check on x86_64 is correct and therefore is

Typo: s/stack/the stack/

> not affected by the problem:
> 
> | RA == BASE + (RD=NARGS+1)*8 + framesize * 8 +8 > maxstack
> 
> The patch partially fixes aforementioned issue by

Typo: s/aforementioned/the aforementioned/

> bumping LJ_STACK_EXTRA by 1 to give a space to write
> the entire frame link and fixing a number of last

Typo: s/a number/the number/

I'm not get this part. I suggest rephrasing it like the following:

| The patch partially fixes the aforementioned issue by bumping
| LJ_STACK_EXTRA by 1 to give a space to the entire frame link for a
| vararg function as the __newindex metamethod.


> free slot in the stack (LJ_FR2 summand adjustment).
> 
> A fixup for a number of required slots in `call_init()` was added
> for consistency with non-gc64 flavor.

Typo: s/gc64/GC64/
I would also add: "The check is too strict, so this can't lead to any
crash."

> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#12134
> ---
>  src/lj_def.h                                  |  2 +-
>  src/lj_dispatch.c                             |  2 +-
>  src/vm_arm64.dasc                             |  1 +
>  src/vm_mips64.dasc                            |  1 +
>  ...048-fix-stack-checks-vararg-calls.test.lua | 53 +++++++++++++++++++
>  5 files changed, 57 insertions(+), 2 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
> 
> diff --git a/src/lj_def.h b/src/lj_def.h
> index a5bca6b0..7e4f251e 100644
> --- a/src/lj_def.h
> +++ b/src/lj_def.h
> @@ -69,7 +69,7 @@ typedef unsigned int uintptr_t;
>  #define LJ_MAX_UPVAL	60		/* Max. # of upvalues. */
>  
>  #define LJ_MAX_IDXCHAIN	100		/* __index/__newindex chain limit. */
> -#define LJ_STACK_EXTRA	(5+2*LJ_FR2)	/* Extra stack space (metamethods). */
> +#define LJ_STACK_EXTRA	(5+3*LJ_FR2)	/* Extra stack space (metamethods). */
>  
>  #define LJ_NUM_CBPAGE	1		/* Number of FFI callback pages. */
>  
> diff --git a/src/lj_dispatch.c b/src/lj_dispatch.c
> index a44a5adf..431cb3c2 100644
> --- a/src/lj_dispatch.c
> +++ b/src/lj_dispatch.c
> @@ -453,7 +453,7 @@ static int call_init(lua_State *L, GCfunc *fn)
>      int numparams = pt->numparams;
>      int gotparams = (int)(L->top - L->base);
>      int need = pt->framesize;
> -    if ((pt->flags & PROTO_VARARG)) need += 1+gotparams;
> +    if ((pt->flags & PROTO_VARARG)) need += 1+LJ_FR2+gotparams;
>      lj_state_checkstack(L, (MSize)need);
>      numparams -= gotparams;
>      return numparams >= 0 ? numparams : 0;

Let's add an additional test for this part of code (since we don't have
any). It may be taken from [1]. It doesn't fail now, but we may cover
this branch more precise.

> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
> index c5f0a7a7..cf8e575a 100644
> --- a/src/vm_arm64.dasc
> +++ b/src/vm_arm64.dasc

<snipped>

> diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
> index da187a7a..6c2975b4 100644
> --- a/src/vm_mips64.dasc
> +++ b/src/vm_mips64.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
> new file mode 100644
> index 00000000..d471d41e
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
> @@ -0,0 +1,53 @@
> +local tap = require('tap')
> +
> +-- A test file to demonstrate a stack overflow in `pcall()` in

I would rephrase it like "demonstrate a crash due to Lua stack
out-of-bounds access".

> +-- some cases, see below testcase descriptions.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/1048.
> +local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})

I suppose there is no need in JIT here. This skipcond may be removed.

> +
> +test:plan(2)
> +
> +-- The testcase demonstrate a segmentation fault due to stack

Typo: s/testcase demonstrate/test case demonstrates/

> +-- overflow by recursive calling `pcall()`. The functions are
> +-- vararg because stack check in BC_IFUNCV is off by one on ARM64

Typo: s/stack/the stack/

> +-- and MIPS64 without the patch.
> +local function prober_1(...) -- luacheck: no unused
> +  -- Any fast function can be used as metamethod, but `type` is
> +  -- convenient here because it works fast and can be used with
> +  -- any data type. Lua function cannot be used since it
> +  -- will check the stack on each invocation.

Please add the comment, that we need to check using of the correct
value LJ_STACK_EXTRA slots (5+3*LJ_FR2) = 8 for GC64 mode.

> +  pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, type, 0)
> +end
> +
> +local function looper(prober, n, ...)
> +  prober(...)
> +  return looper(prober, n + 1, n, ...)
> +end
> +
> +pcall(coroutine.wrap(looper), prober_1, 0)
> +
> +test:ok(true, 'no stack overflow with recursive pcall')
> +
> +-- The testcase demonstrate a segmentation fault due to stack

Typo: s/testcase demonstrate/test case demonstrates/

> +-- overflow when `pcall()` is used as `__newindex` metamethod.
> +-- The function is vararg because stack check in BC_IFUNCV is off

Typo: s/stack/the stack/

> +-- by one on ARM64 and MIPS64 without the patch.
> +
> +-- Any fast function can be used as metamethod, but `type` is

Typo: s/metamethod/a metamethod/

> +-- convenient here because it works fast and can be used with
> +-- any data type. Lua function cannot be used since it

Typo: s/Lua/The Lua/

> +-- will check the stack on each invocation.
> +local t = setmetatable({}, { __newindex = pcall, __call = type })
> +
> +local function prober_2(...) -- luacheck: no unused
> +  -- Invokes `pcall(t, t, t)`.
> +  t[t] = t
> +end
> +
> +pcall(coroutine.wrap(looper), prober_2, 0)
> +
> +test:ok(true, 'no stack overflow with metamethod')
> +
> +test:done(true)
> -- 
> 2.43.0
> 

[1]: https://github.com/LuaJIT/LuaJIT/issues/1402#issue-3569942423

-- 
Best regards,
Sergey Kaplun