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

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

[-- Attachment #1: Type: text/plain, Size: 4338 bytes --]

Hi, Sergey,

thanks for review! Please consider my three comments below.

Sergey

On 10/27/25 11:16, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
> Please consider my comments below.
> Also, please send the next version via v2 series to simplify the
> review.
>
> On 23.09.25, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for review! Please see my comments below.
>>
>> Sergey
>>
>> On 9/1/25 16:07, Sergey Kaplun via Tarantool-patches wrote:
>>> Hi, Sergey!
>>> Thanks for the patch!
>>> Please consider my comments below.
>>>
>>> On 27.08.25, Sergey Bronnikov wrote:
> <snipped>
>
>>>> Sergey Bronnikov:
>>>> * added the description and the test for the problem
>>>>
>>>> Part of tarantool/tarantool#11691
>>>> ---
>>>>    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 | 56 +++++++++++++++++++
>>>>    5 files changed, 60 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
>>> <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
>>>> @@ -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;
>>> I can't see the test related to this change. Not `prober_1()` nor
>>> `prober_2()` lead to the assertion failure for x86_64 or aarch64 without
>>> it.

The LJ_FR2 check was added for consistency with non-gc64 flavor, the 
commit message was updated

 >    A fixup for a number of required slots in `call_init()` was added
 >   for consistency with non-gc64 flavor.

Also, there is an issue [1] about inconsistencies in stack checks.

1. https://github.com/LuaJIT/LuaJIT/issues/1402

>> Please check again. Both testcases trigger segfault on AArch64 (odroid).

> Double checked:
> | root@odroid:/home/skaplun/lj-1048-review# git diff
> | diff --git a/src/lj_dispatch.c b/src/lj_dispatch.c
> | index 431cb3c2..a44a5adf 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+LJ_FR2+gotparams;
> | +    if ((pt->flags & PROTO_VARARG)) need += 1+gotparams;
> |      lj_state_checkstack(L, (MSize)need);
> |      numparams -= gotparams;
> |      return numparams >= 0 ? numparams : 0;
> | Test project /home/skaplun/lj-1048-review
> |     Start 118: test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
> | 1/1 Test #118: test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua ...   Passed    3.38 sec
> |
> | 100% tests passed, 0 tests failed out of 1
> |
> | Label Time Summary:
> | tarantool-tests    =   3.38 sec*proc (1 test)
> |
> | Total Test time (real) =   3.42 sec
>
> <snipped>
>
>>>> +-- patch.
>>>> +local function prober_1(...) -- luacheck: no unused
>>>> +  pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, pairs, {})
>>>> +end
>>> Why do we want to use probber_1 here? Why is this different from the
>>> second example? Only because of the metamethods?
`prober_1` triggers the issue by using recursive (p)call
> Still need an explanation.
>
>>> If we want to keep it, please describe why we need at least 9 pcall-s.
>> As I got right, exactly this number of pcall's is needed to trigger a
>> stack overflow.
> Yes, but why 9 is minimum number of pcall's when the issue is reproduced?
>
> <snipped>

The number depends on a previous value of LJ_STACK_EXTRA.

LJ_STACK_EXTRA, is an "overlay" on top of the stack, and for a buffer 
overflow
you need at least 8 + 1 frames to write slots above this 
"overlay". These pcalls generates additional frames.

[-- Attachment #2: Type: text/html, Size: 7045 bytes --]