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

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

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

Hi, Sergey,

thanks for review! See comments below. The branch was force-pushed.

Sergey

On 3/12/26 12:36, Sergey Kaplun via Tarantool-patches wrote:
> 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/
Fixed, thanks!
>> 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.

Right, fixed:

--- a/src/luajit_lldb.py
+++ b/src/luajit_lldb.py
@@ -833,7 +833,7 @@ def dump_stack(L, base=None, top=None):
      top = top or L.top
      stack = mref(TValuePtr, L.stack)
      maxstack = mref(TValuePtr, L.maxstack)
-    red = 5 + 2 * LJ_FR2
+    red = 5 + 3 * LJ_FR2

      dump = [
          '{padding} Red zone: {nredslots: >2} slots {padding}'.format(

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

Ah, right, I've overlooked it is a LuaJIT issue, not Tarantool. Thanks!

Renamed.

>> 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.
>
Updated.
>> 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.
renamed
>
>>   ...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.
ok, renamed once again
>
>> 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.
Updated.
>
> The test file to verify correctness of stack size check during recording of vararg functions.
>> +-- See alsohttps://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.

renamed

--- a/test/tarantool-tests/lj-1402-vararg-stkov-check-gc64.test.lua
+++ b/test/tarantool-tests/lj-1402-vararg-stkov-check-gc64.test.lua
@@ -1,15 +1,16 @@
  local tap = require('tap')

--- A test file to demonstrate a probably quite strict stack
--- check for vararg functions in call_init.
+-- 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({
+local test = 
tap.test('lj-1402-vararg-stkov-check-gc64.test.lua'):skipcond({
    ['Test requires JIT enabled'] = not jit.status(),
  })


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

Updated:

--- a/test/tarantool-tests/lj-1402-vararg-stkov-check-gc64.test.lua
+++ b/test/tarantool-tests/lj-1402-vararg-stkov-check-gc64.test.lua
@@ -1,15 +1,16 @@
  local tap = require('tap')

--- A test file to demonstrate a probably quite strict stack
--- check for vararg functions in call_init.
+-- 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({
+local test = 
tap.test('lj-1402-vararg-stkov-check-gc64.test.lua'):skipcond({
    ['Test requires JIT enabled'] = not jit.status(),
  })

test:plan(1)

-local function vararg(...) -- luacheck: no unused
+-- luacheck: no unused
+local function vararg(...)
    -- None.
  end

>
>> +  -- None.
>> +end
>> +
>> +-- Make compilation aggressive.
> Excess comment. It's quite general approach in our tests.


Updated:

test:plan(1)

-local function vararg(...) -- luacheck: no unused
+-- luacheck: no unused
+local function vararg(...)
    -- None.
  end


>> +jit.opt.start("hotloop=1")
> Typo: s/"/'/g
>
>> +
  end

--- Make compilation aggressive.
-jit.opt.start("hotloop=1")
+jit.opt.start('hotloop=1')

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

> 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.
--- Make compilation aggressive.
-jit.opt.start("hotloop=1")
+jit.opt.start('hotloop=1')

+-- 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
    local _, _, _, _, _, _, _, _, _, _

>> +local function caller()
>> +  -- luacheck: push no unused
> Lets drop this luacheck suppression, see the comment above.

Updated:

+-- 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
    local _, _, _, _, _, _, _, _, _, _
    local _, _, _, _, _, _, _, _, _, _
    local _, _, _, _, _, _, _, _, _, _
-  -- luacheck: pop
    local n = 1
    while n < 3 do
      vararg()

>> +  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)()
>
@@ -29,7 +29,7 @@ local function caller()
    end
  end

-pcall(coroutine.wrap(caller))
+coroutine.wrap(caller)()

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

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

Updated:


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

test:done(true)

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

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