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

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

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

Hi, Sergey,

thanks for review!

Sergey

On 2/11/26 13:24, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On 10.12.25, Sergey Bronnikov wrote:
>> From: Mike Pall <mike>
>>
>> Analyzed by Peter Cawley.
>>
>> (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
>>
>> In the previous commit ("LJ_FR2: Fix stack checks in vararg calls.")
>> stack overflow for vararg functions and metamethod invocations
>> was fixed partially and there are still cases where stack overflow
>> happens, see comments in the test. The patch fixes the issue by
> Please describe the issue regardless previous commit. Just mentioned
> missing stack checks for `pcall()`, `xpcall()` is enough.

Updated commit message:


     Add stack check to pcall/xpcall.

     Analyzed by Peter Cawley.

     (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)

     The patch adds the stack check to fast functions `pcall()` and
     `xpcall()`.

     Sergey Bronnikov:
     * added the description and the test for the problem

     Part of tarantool/tarantool#12134


>> adding the stack check to fast functions `pcall()` and `xpcall()`.
>>
>> 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 | 31 ++++++++++++++++++-
>>   8 files changed, 86 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc
>> index 7095e660..efe9dcb2 100644
>> --- a/src/vm_arm.dasc
>> +++ b/src/vm_arm.dasc
> <snipped>
>
>> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
>> index cf8e575a..53ff7162 100644
>> --- a/src/vm_arm64.dasc
>> +++ b/src/vm_arm64.dasc
> <snipped>
>
>> diff --git a/src/vm_mips.dasc b/src/vm_mips.dasc
>> index 32caabf7..69d09d52 100644
>> --- a/src/vm_mips.dasc
>> +++ b/src/vm_mips.dasc
> <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 change is incorrect.
> It wipes out the first patch in the series.

Sorry, my bad.

--- 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)
-  |  daddiu NARGS8:RC, NARGS8:RC, -16
+  |  daddiu NARGS8:TMP0, NARGS8:RC, -16
    |   ld CARG2, 8(BASE)
    |    bltz NARGS8:TMP0, ->fff_fallback
    |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)

>> -  |  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
>>     |   ld CARG2, 8(BASE)
>>     |    bltzNARGS8:TMP0, ->fff_fallback
>>     |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)
>> diff --git a/src/vm_ppc.dasc b/src/vm_ppc.dasc
>> index 980ad897..f2ea933b 100644
>> --- a/src/vm_ppc.dasc
>> +++ b/src/vm_ppc.dasc
> <snipped>
>
>> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
>> index d5296759..141f5f82 100644
>> --- a/src/vm_x64.dasc
>> +++ b/src/vm_x64.dasc
> <snipped>
>
>> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
>> index b043b830..1ba5abce 100644
>> --- a/src/vm_x86.dasc
>> +++ b/src/vm_x86.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
>> index d471d41e..b135042b 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
>> @@ -1,4 +1,5 @@
>>   local tap = require('tap')
>> +local ffi = require('ffi')
>>   
>>   -- A test file to demonstrate a stack overflow in `pcall()` in
>>   -- some cases, see below testcase descriptions.
>> @@ -7,7 +8,7 @@ local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({
>>     ['Test requires JIT enabled'] = not jit.status(),
>>   })
>>   
>> -test:plan(2)
>> +test:plan(4)
> This patch covers only `pcall()` cases, please add the same tests for
> `xpcall()` (I suppose the most simple is `xpcall()` as `__call`
> metamethod).

Added:

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 0ce3c756..8f45941c 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
@@ -6,7 +6,7 @@ local ffi = require('ffi')
  -- See also https://github.com/LuaJIT/LuaJIT/issues/1048.
  local test = tap.test('lj-1048-fix-stack-checks-vararg-calls')

-test:plan(4)
+test:plan(5)

  -- The test case demonstrates a segmentation fault due to stack
  -- overflow by recursive calling `pcall()`. The functions are
@@ -51,12 +51,16 @@ pcall(coroutine.wrap(looper), prober_2, 0)

test:ok(true, 'no stack overflow with metamethod')

--- The testcase demonstrates a stack overflow in
+-- The testcases demonstrates a stack overflow in
  -- `pcall()`/xpcall()` triggered using metamethod `__call`.

  t = coroutine.wrap(setmetatable)({}, { __call = pcall })

-test:ok(true, 'no stack overflow with metamethod __call')
+test:ok(true, 'no stack overflow with metamethod __call with pcall()')
+
+t = coroutine.wrap(setmetatable)({}, { __call = xpcall })
+
+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

>>   
>>   -- The testcase demonstrate a segmentation fault due to stack
>>   -- overflow by recursive calling `pcall()`. The functions are
>> @@ -50,4 +51,32 @@ pcall(coroutine.wrap(looper), prober_2, 0)
>>   
>>   test:ok(true, 'no stack overflow with metamethod')
>>   
>> +-- The testcase demonstrate a stack overflow in
> Typo: s/demonstrate/demonstrates/
Fixed.
>> +-- `pcall()`/xpcall()` triggered using metamethod `__call`.
>> +
>> +t = setmetatable({}, { __call = pcall })()
> It's better to do this at the separate Lua stack, i.e. inside
> `coroutine.wrap()`.

Updated.

  -- `pcall()`/xpcall()` triggered using metamethod `__call`.

-t = setmetatable({}, { __call = pcall })()
+t = coroutine.wrap(setmetatable)({}, { __call = pcall })

>> +
>> +test:ok(true, 'no stack overflow with metamethod __call')
>> +
>> +-- The testcase demonstrate a stack overflow in
> Typo: s/demonstrate/demonstrates/
Fixed.
>> +-- `pcall()`/`xpcall()` similar to the first testcase, but it is
>> +-- triggered using `unpack()`.
>> +
>> +t = {}
>> +local function f()
>> +  return pcall(unpack(t))
>> +end
>> +
> Please explain the amount of necessary iterations of calls.

After a small research we found that issue cannot be reproduced in 
LuaJIT GC32 flavors.

N_ITERATION should be equal to at least 200 for reproducing.

The problem reproduced better under Valgrind than ASAN.


>> +local N_ITERATIONS = 100
>> +if ffi.abi('gc64') then
>> +  N_ITERATIONS = 180
>> +end
>> +
>> +for i = 1, N_ITERATIONS do
>> +  t[i], t[i + 1], t[i + 2] = pcall, pairs, {}
> Let's use `type` here instead of pairs.
Updated.
>> +  coroutine.wrap(f)()
>> +end
>> +
>> +test:ok(true, 'no stack overflow with unpacked pcalls')
>> +
>>   test:done(true)
>> -- 
>> 2.43.0
>>

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