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

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

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

Hi, Sergey,

thanks for review!

On 2/11/26 11:30, Sergey Kaplun via Tarantool-patches wrote:
> 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/
Fixed.
>
>> function on ARM64 and MIPS64 in LJ_FR2 mode. This happen
> Typo: s/happen/happens/
Fixed.
>
>> because stack check in BC_IFUNCV is off by one on these
> Typo: s/stack/the stack/
Fixed.
>
>> 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/
Fixed.
>
>> 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/
Fixed.
>
>> 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/
Fixed.
>
> 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.
Updated.
>
>
>> 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/
Fixed.
> I would also add: "The check is too strict, so this can't lead to any
> crash."
Updated.
>> 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.

Don't get what do you mean.

true branch in gc32 is covered by the following tests:

test/LuaJIT-tests
test/PUC-Rio-Lua-5.1-tests
test/tarantool-c-tests/lj-1087-vm-handler-call.c_test
test/tarantool-tests/fix-ff-select-recording.test.lua
test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua
test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
test/tarantool-tests/fix-slots-overflow-for-varg-record.test.lua
test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua
test/tarantool-tests/lj-1024-varg-maxslot.test.lua
test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua
test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
test/tarantool-tests/lj-1164-record-meta-concat-varg-pcall.test.lua
test/tarantool-tests/lj-1295-bad-renames-for-sunk-values.test.lua
test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
test/tarantool-tests/lj-704-bc-varg-use-def.test.lua

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

Updated:

--- 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
@@ -1,7 +1,7 @@
  local tap = require('tap')

--- A test file to demonstrate a stack overflow in `pcall()` in
--- some cases, see below testcase descriptions.
+-- A test file to demonstrate a crash due to Lua stack
+-- out-of-bounds access, 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(),

>
>> +-- some cases, see below testcase descriptions.
>> +-- See alsohttps://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.
Removed.
>> +
>> +test:plan(2)
>> +
>> +-- The testcase demonstrate a segmentation fault due to stack
> Typo: s/testcase demonstrate/test case demonstrates/
Updated.
>
>> +-- 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/
Updated.
>
>> +-- 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.

Updated:

  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.
+  -- will check the stack on each invocation. 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


>
>> +  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/
Updated.
>
>> +-- 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/
Updated.
>
>> +-- 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/
Fixed.
>
>> +-- 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/
Fixed.
>
>> +-- 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
>

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