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

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

Hi, Sergey!
Thanks for the patch!

LGTM, after fixing my nits below.
Please add the iterational diff for the fixes.

On 12.03.26, Sergey Bronnikov wrote:
> From: Mike Pall <mike>
> 
> Thanks to Peter Cawley.
> 
> (cherry picked from commit d1a2fef8a8f53b0055ee041f7f63d83a27444ffa)
> 
> Stack overflow can cause a segmentation fault in a vararg
> function on ARM64 and MIPS64 in LJ_FR2 mode. This happens
> because the stack check in BC_IFUNCV is off by one on these
> platforms without the patch. The original stack check
> for ARM64 and MIPS64 was incorrect:
> 
> | RA == BASE + (RD=NARGS)*8 + framesize * 8 >= maxstack
> 
> while the stack check on x86_64 is correct and therefore is
> not affected by the problem:
> 
> | RA == BASE + (RD=NARGS+1)*8 + framesize * 8 +8 > maxstack

Typo: s/ +8/ + 8/

> 
> 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.
> 
> A fixup for a number of required slots in `call_init()` was added
> for consistency with non-GC64 flavor. The check is too strict, so
> this can't lead to any crash.
> 
> This patch also corrects the number of redzone slots in
> luajit-gdb.py to match the updated LJ_STACK_EXTRA and adds the test

luajit_lldb.py should be updated as well.

> <gh-1402-call_init-regression.test.lua> that will help to avoid

gh- prefix is for the Tarantool issue tracker, use lj- for LuaJIT issue
tracker.

> a regression in the future, see details in [1].

Just mention details here 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.
|
| A fixup for a number of required slots in `call_init()` was added for
| consistency with the non-GC64 flavor. The check is too strict (if
| comparing the corresponding checks in the VM BC_IFUNCV), so this can't
| lead to any crash. To avoid possible regression in the future the
| corresponding test is added.
|
| This patch also corrects the number of redzone slots in luajit-gdb.py
| and luajit_lldb.py to match the updated LJ_STACK_EXTRA.

> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#12134
> 
> 1. https://github.com/LuaJIT/LuaJIT/issues/1402

Please, don't mention the issue during backporting, to avoid messing the
issue tracker.

> ---
>  src/lj_def.h                                  |  2 +-
>  src/lj_dispatch.c                             |  2 +-
>  src/luajit-gdb.py                             |  2 +-
>  src/vm_arm64.dasc                             |  1 +
>  src/vm_mips64.dasc                            |  1 +
>  .../gh-1402-call_init-regression.test.lua     | 36 +++++++++++++

gh- prefix is for the Tarantool issue tracker, use lj- for LuaJIT issue
tracker.

>  ...048-fix-stack-checks-vararg-calls.test.lua | 53 +++++++++++++++++++
>  7 files changed, 94 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-1402-call_init-regression.test.lua
>  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

<snipped>

> 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

<snipped>

> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> index 0ae2a6e0..dab07b35 100644
> --- a/src/luajit-gdb.py
> +++ b/src/luajit-gdb.py

<snipped>

> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
> index 6600e226..5ef37243 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/gh-1402-call_init-regression.test.lua b/test/tarantool-tests/gh-1402-call_init-regression.test.lua

Please, avoid _ in the file names, lets name it like:

lj-1402-vararg-stkov-check-gc64.test.lua

Same for the name of the test.

> new file mode 100644
> index 00000000..b20f9e39
> --- /dev/null
> +++ b/test/tarantool-tests/gh-1402-call_init-regression.test.lua
> @@ -0,0 +1,36 @@
> +local tap = require('tap')
> +
> +-- A test file to demonstrate a probably quite strict stack
> +-- check for vararg functions in call_init.

This is not about quite strict stack check. We need this to test the
behaviour of the LuaJIT while recording the vararg function. Let's
rephrase like the following:

| -- The test file to verify correctness of stack size check during
| -- recording of vararg functions.

The test file to verify correctness of stack size check during recording of vararg functions.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/1402
> +local test = tap.test('gh-1402-call_init-regression.test.lua'):skipcond({

gh- prefix is for the Tarantool issue tracker, use lj- for LuaJIT issue
tracker.

> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local function vararg(...) -- luacheck: no unused

Let's use this comment before the vararg declaration.
It helps with the _ below as well.

> +  -- None.
> +end
> +
> +-- Make compilation aggressive.

Excess comment. It's quite general approach in our tests.

> +jit.opt.start("hotloop=1")

Typo: s/"/'/g

> +

Please add the following comment:

| -- This function utilizes the exact amount of stack slots
| -- to cause the stack reallocation during `call_init()` in the
| -- GC64 mode.

> +local function caller()
> +  -- luacheck: push no unused

Lets drop this luacheck suppression, see the comment above.

> +  local _, _, _, _, _, _, _, _, _, _
> +  local _, _, _, _, _, _, _, _, _, _
> +  local _, _, _, _, _, _, _, _, _, _
> +  -- luacheck: pop
> +  local n = 1
> +  while n < 3 do
> +    vararg()
> +    n = n + 1
> +  end
> +end
> +
> +pcall(coroutine.wrap(caller))

The pcall is excess lets do it without it:
| coroutine.wrap(caller)()

> +
> +test:ok(true, 'no assertion for vararg functions in call_init')

Just mention 'no assertion failure' (this assertion isn't in the
`call_init()`, but during recording in `rec_check_slots()`).

> +
> +test:done(true)
> 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..3a8ad63d
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun