Re: [Tarantool-patches] [PATCH luajit 3/3][v3] Add stack check to pcall/xpcall.

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

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

Hi, Sergey,

I've updated N_ITERATIONS and force-pushed the branch.

Sergey

On 3/12/26 20:19, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
>
> LGTM, after fixing the last nit below.
>
> On 12.03.26, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for review! See my comments below.
>>
>> Sergey
>>
>> On 3/12/26 13:16, Sergey Kaplun via Tarantool-patches wrote:
>>> Hi, Sergey!
>>> Thanks for the patch!
>>> Please, fix my comments below.
>>>
>>> Don't forget to add the corresponding iterative changes.
>>>
>>> On 12.03.26, Sergey Bronnikov wrote:
>>>> From: Mike Pall <mike>
>>>>
>>>> Analyzed by Peter Cawley.
>>>>
>>>> (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
>>>>
>>>> The patch adds the stack check to fast functions `pcall()` and
>>>> `xpcall()`.
>>> Please add more verbose description:
>>>
>>> | (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
>>> |
>>> | The `pcall()` and `xpcall()` calls in GC64 mode require 2 slots. This
>>> | means that all arguments should be moved up during emitting of the frame
>>> | link to the stack. Hence, this may cause stack overflow without the
>>> | corresponding check.
>>> |
>>> | This patch adds the corresponding checks to the VM. Non-GC64 VMs are
>>> | updated as well for the consistency.
>> Updated
>>>> Sergey Bronnikov:
>>>> * added the description and the test for the problem
>>>>
>>>> Part of tarantool/tarantool#12134
>>>> ---
>>>>    src/vm_arm.dasc                               |  7 ++++
>>>>    src/vm_arm64.dasc                             |  8 +++++
>>>>    src/vm_mips.dasc                              | 10 +++++-
>>>>    src/vm_mips64.dasc                            | 14 ++++++--
>>>>    src/vm_ppc.dasc                               |  9 +++++
>>>>    src/vm_x64.dasc                               |  6 ++++
>>>>    src/vm_x86.dasc                               |  6 ++++
>>>>    ...048-fix-stack-checks-vararg-calls.test.lua | 35 ++++++++++++++++++-
>>>>    8 files changed, 90 insertions(+), 5 deletions(-)
>>>>
> <snipped>
>
>>>> diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
>>>> index 6c2975b4..4e60ee07 100644
>>>> --- a/src/vm_mips64.dasc
>>>> +++ b/src/vm_mips64.dasc
>>>> @@ -1418,8 +1418,12 @@ static void build_subroutines(BuildCtx *ctx)
>>>>      |//-- Base library: catch errors ----------------------------------------
>>>>      |
>>>>      |.ffunc pcall
>>>> +  |  ld TMP1, L->maxstack
>>>> +  |  daddu TMP2, BASE,NARGS8:RC
>>>> +  |  sltu AT, TMP1, TMP2
>>>> +  |  bnez AT, ->fff_fallback
>>>> +  |.  lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
>>>>      |daddiuNARGS8:RC,NARGS8:RC, -8
>>>> -  |  lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
>>>>      |bltzNARGS8:RC, ->fff_fallback
>>>>      |.   move TMP2, BASE
>>>>      |   daddiu BASE, BASE, 16
>>>> @@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)
>>>>      |.  nop
>>>>      |
>>>>      |.ffunc xpcall
>>>> -  |daddiuNARGS8:TMP0,NARGS8:RC, -16
>>> This neglets the first patch in the series. See the comment below.
>>>
>>>> -  |  ld CARG1, 0(BASE)
>>>> +  |  ld TMP1, L->maxstack
>>>> +  |  daddu TMP2, BASE,NARGS8:RC
>>>> +  |  sltu AT, TMP1, TMP2
>>>> +  |  bnez AT, ->fff_fallback
>>>> +  |.  ld CARG1, 0(BASE)
>>>> +  |daddiuNARGS8:RC,NARGS8:RC, -16
>>> This line is incorrect. This neglets the 1st patch in the series.
>>>
>>> It should be
>>> | |daddiuNARGS8:TMP0,NARGS8:RC, -16
>> Right. However, probably we should leave this line near ".ffunc xpcall".
>> What do you think?
> Why? This break the `maxstack` check (since the RC is differs before the
> addition with TMP2).

Ok, let's leave as is, seems I took a look on intermediate version, 
before applying changes you requested on review.

The current patch looks fine for me:

@@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)
    |.  nop
    |
    |.ffunc xpcall
+  |  ld TMP1, L->maxstack
+  |  daddu TMP2, BASE, NARGS8:RC
+  |  sltu AT, TMP1, TMP2
+  |  bnez AT, ->fff_fallback
+  |.  ld CARG1, 0(BASE)
    |  daddiu NARGS8:TMP0, NARGS8:RC, -16
-  |  ld CARG1, 0(BASE)
    |   ld CARG2, 8(BASE)
    |    bltz NARGS8:TMP0, ->fff_fallback
    |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)

>
> See the latest LuaJIT version:
> https://github.com/LuaJIT/LuaJIT/blob/659a61693aa3b87661864ad0f12eee14c865cd7f/src/vm_mips64.dasc#L1450
>
>> Now updated as the following:
>>
>> --- a/src/vm_mips64.dasc
>> +++ b/src/vm_mips64.dasc
>> @@ -1449,7 +1449,7 @@ static void build_subroutines(BuildCtx *ctx)
>>      |  sltu AT, TMP1, TMP2
>>      |  bnez AT, ->fff_fallback
>>      |.  ld CARG1, 0(BASE)
>> -  |  daddiuNARGS8:RC,NARGS8:RC, -16
>> +  |  daddiuNARGS8:TMP0,NARGS8:RC, -16
>>      |   ld CARG2, 8(BASE)
>>      |    bltzNARGS8:TMP0, ->fff_fallback
>>      |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)
>>
>>>>      |   ld CARG2, 8(BASE)
>>>>      |bltzNARGS8:TMP0, ->fff_fallback
>>>>      |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)
> <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
>>>> index 3a8ad63d..ad8b151b 100644
>>>> --- 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
>>>> @@ -5,7 +5,7 @@ local tap = require('tap')
>>>>    -- Seealsohttps://github.com/LuaJIT/LuaJIT/issues/1048.
>>>>    local test = tap.test('lj-1048-fix-stack-checks-vararg-calls')
>>>>    
>>>> -test:plan(2)
>>>> +test:plan(5)
>>>>    
>>>>    -- The test case demonstrates a segmentation fault due to stack
>>>>    -- overflow by recursive calling `pcall()`. The functions are
>>>> @@ -50,4 +50,37 @@ pcall(coroutine.wrap(looper), prober_2, 0)
>>>>    
>>>>    test:ok(true, 'no stack overflow with metamethod')
>>>>    
>>>> +-- The testcases demonstrates a stack overflow in
>>>> +-- `pcall()`/xpcall()` triggered using metamethod `__call`.
>>>> +
>>>> +t = coroutine.wrap(setmetatable)({}, { __call = pcall })
>>> I've meant the following:
>>>
>>> | t = setmetatable({}, { __call = pcall })
>>> | coroutine.wrap(function() t() end)()
>>>
>> Updated
>>
>> @@ -53,7 +53,8 @@test:ok(true, 'no stack overflow with metamethod')
>>    -- The testcases demonstrates a stack overflow in
>>    -- `pcall()`/xpcall()` triggered using metamethod `__call`.
>>
>> -t = coroutine.wrap(setmetatable)({}, { __call = pcall })
>> +t = setmetatable({}, { __call = pcall })
>> +coroutine.wrap(function() t() end)()
>>
>> test:ok(true, 'no stack overflow with metamethod __call with pcall()')
>>
>>
>>>> +
>>>> +test:ok(true, 'no stack overflow with metamethod __call with pcall()')
>>>> +
>>>> +t = coroutine.wrap(setmetatable)({}, { __call = xpcall })
>>> I've meant the following:
>>>
>>> | t = setmetatable({}, { __call = xpcall })
>>> | coroutine.wrap(function() t() end)()
>>>
>>> But this won't work since the second amount of xpcall must be the
>>> function. So, this test case is invalid. We must to duplicate the second
>>> approach with `xpcall()`
>>>
>>> This works fine.
>>> | LUA_PATH="src/?.lua;;" gdb --args src/luajit -e '
>>> | local t = {}
>>> | local function xpcall_wrapper()
>>> |   return xpcall(unpack(t))
>>> | end
>>> |
>>> | local N_ITERATIONS = 200
>>> |
>>> | for i = 1, N_ITERATIONS do
>>> |   t[i], t[i + 1], t[i + 2] = xpcall, type, {}
>>> |   coroutine.wrap(xpcall_wrapper)()
>>> | end
>>> | '
>> Updated:
>>
>> 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
>> index 6395dfaa..825568f9 100644
>> --- 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
>> @@ -58,7 +58,17 @@ coroutine.wrap(function() t() end)()
>>
>> test:ok(true, 'no stack overflow with metamethod __call with pcall()')
>>
>> -t = coroutine.wrap(setmetatable)({}, { __call = xpcall })
>> +t = {}
>> +local function xpcall_wrapper()
>> +  return xpcall(unpack(t))
>> +end
>> +
>> +local N_ITERATIONS_1 = 200
> Why do we need two variables with the same value of iterations?
> Let's use N_ITERATIONS with the comment for xpcall and pcall.

Updated:

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
index 825568f9..036d53e9 100644
--- 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
@@ -63,9 +63,13 @@ local function xpcall_wrapper()
    return xpcall(unpack(t))
  end

-local N_ITERATIONS_1 = 200
+-- The problems are only reproduced on LuaJIT GC64 and is better
+-- reproduced under Valgrind than AddressSanitizer. The chosen
+-- value was found experimentally and always results in an attempt
+-- to write beyond the allocated memory.
+local N_ITERATIONS = 200

-for i = 1, N_ITERATIONS_1 do
+for i = 1, N_ITERATIONS do
    t[i], t[i + 1], t[i + 2] = xpcall, type, {}
    coroutine.wrap(xpcall_wrapper)()
  end
@@ -81,13 +85,7 @@ local function pcall_wrapper()
    return pcall(unpack(t))
  end

--- The problem is only reproduced on LuaJIT GC64 and is better
--- reproduced under Valgrind than AddressSanitizer. The chosen
--- value was found experimentally and always results in an attempt
--- to write beyond the allocated memory.
-local N_ITERATIONS_2 = 200
-
-for i = 1, N_ITERATIONS_2 do
+for i = 1, N_ITERATIONS do
    t[i], t[i + 1], t[i + 2] = pcall, type, {}
    coroutine.wrap(pcall_wrapper)()
  end

>
>> +
>> +for i = 1, N_ITERATIONS_1 do
>> +  t[i], t[i + 1], t[i + 2] = xpcall, type, {}
>> +  coroutine.wrap(xpcall_wrapper)()
>> +end
>>
>> test:ok(true, 'no stack overflow with metamethod __call with xpcall()')
>>
>> @@ -67,19 +77,19 @@test:ok(true, 'no stack overflow with metamethod
>> __call with xpcall()')
>>    -- triggered using `unpack()`.
>>
>>    t = {}
>> -local function f()
>> +local function pcall_wrapper()
>>      return pcall(unpack(t))
>>    end
>>
>> --- The problem is only reproduced on LuaJIT GC64 and is best
>> +-- The problem is only reproduced on LuaJIT GC64 and is better
>>    -- reproduced under Valgrind than AddressSanitizer. The chosen
>>    -- value was found experimentally and always results in an attempt
>>    -- to write beyond the allocated memory.
>> -local N_ITERATIONS = 200
>> +local N_ITERATIONS_2 = 200
>>
>> -for i = 1, N_ITERATIONS do
>> +for i = 1, N_ITERATIONS_2 do
>>      t[i], t[i + 1], t[i + 2] = pcall, type, {}
>> -  coroutine.wrap(f)()
>> +  coroutine.wrap(pcall_wrapper)()
>>    end
>>
>> test:ok(true, 'no stack overflow with unpacked pcalls')
>>
>>>> +
>>>> +test:ok(true, 'no stack overflow with metamethod __call with xpcall()')
>>>> +
>>>> +-- The testcase demonstrates a stack overflow in
>>>> +-- `pcall()`/`xpcall()` similar to the first testcase, but it is
>>>> +-- triggered using `unpack()`.
>>>> +
>>>> +t = {}
>>>> +local function f()
> <snipped>
>
>>>> -- 
>>>> 2.43.0
>>>>

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