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

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

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

Hi, Sergey,

thanks for review! Please consider my comments below.

Sergey

On 9/1/25 16:36, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On 27.08.25, Sergey Bronnikov wrote:
>> Analyzed by Peter Cawley.
>>
>> (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
>>
>> In the commit "LJ_FR2: Fix stack checks in vararg calls."
> Minor: In the previous commit ("...")
Fixed.
>
>> stack overflow in `pcall()`/`xpcall()` was fixed partially and
> This handles stack overflow for vararg functions and metamethod
> invocations not `xpcall()/pcall()` (directly).

Updated:

 > 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
 >  adding the stack check to fast functions `pcall()` and `xpcall()`.

>> there are still cases where stack overflow happens, see comments
>> in the test. The patch add stack check to `pcall()` and `xpcall()`.
> Please, mention in the commit message that the issue was fixed by adding
> the stack check to these fast functions.
Added.
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#11691
>> ---
>>   src/vm_arm.dasc                               |  7 +++++
>>   src/vm_arm64.dasc                             |  8 +++++
>>   src/vm_mips.dasc                              | 10 +++++-
>>   src/vm_mips64.dasc                            | 12 +++++--
>>   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, 85 insertions(+), 4 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
>> @@ -1201,8 +1201,11 @@ static void build_subroutines(BuildCtx *ctx)
>>     |//-- Base library: catch errors ----------------------------------------
>>     |
>>     |.ffunc pcall
>> +  |   ldr RB, L->maxstack
>> +  |   add INS, BASE,NARGS8:RC
>>     |  ldrb RA, [DISPATCH, #DISPATCH_GL(hookmask)]
>>     |   cmpNARGS8:RC, #8
>> +  |   cmphs RB, INS
>>     |   blo ->fff_fallback
>>     |  tst RA, #HOOK_ACTIVE		// Remember active hook before pcall.
>>     |   mov RB, BASE
>> @@ -1213,7 +1216,11 @@ static void build_subroutines(BuildCtx *ctx)
>>     |  b ->vm_call_dispatch
>>     |
>>     |.ffunc_2 xpcall
>> +  |   ldr RB, L->maxstack
>> +  |   add INS, BASE,NARGS8:RC
>>     |  ldrb RA, [DISPATCH, #DISPATCH_GL(hookmask)]
>> +  |   cmp RB, INS
>> +  |   blo ->fff_fallback
>>     |  checkfunc CARG4, ->fff_fallback	// Traceback must be a function.
>>     |   mov RB, BASE
>>     |  strd CARG12, [BASE, #8]		// Swap function and traceback.
>> 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
>> @@ -1166,6 +1166,10 @@ static void build_subroutines(BuildCtx *ctx)
>>     |//-- Base library: catch errors ----------------------------------------
>>     |
>>     |.ffunc pcall
>> +  |  ldr TMP1, L->maxstack
>> +  |  add TMP2, BASE,NARGS8:RC
>> +  |  cmp TMP1, TMP2
>> +  |  blo ->fff_fallback
>>     |   cmpNARGS8:RC, #8
>>     |  ldrb TMP0w, GL->hookmask
>>     |   blo ->fff_fallback
>> @@ -1185,6 +1189,10 @@ static void build_subroutines(BuildCtx *ctx)
>>     |  b ->vm_call_dispatch
>>     |
>>     |.ffunc xpcall
>> +  |  ldr TMP1, L->maxstack
>> +  |  add TMP2, BASE,NARGS8:RC
>> +  |  cmp TMP1, TMP2
>> +  |  blo ->fff_fallback
>>     |     ldp CARG1, CARG2, [BASE]
>>     |  ldrb TMP0w, GL->hookmask
>>     |   subsNARGS8:TMP1,NARGS8:RC, #16
>> 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
>> @@ -1382,9 +1382,13 @@ static void build_subroutines(BuildCtx *ctx)
>>     |//-- Base library: catch errors ----------------------------------------
>>     |
>>     |.ffunc pcall
>> +  |   lw TMP1, L->maxstack
>> +  |   addu TMP2, BASE,NARGS8:RC
>>     |  lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
>>     |  beqzNARGS8:RC, ->fff_fallback
>> -  |   move TMP2, BASE
>> +  |.  sltu AT, TMP1, TMP2
>> +  |   bnez AT, ->fff_fallback
>> +  |.   move TMP2, BASE
>>     |   addiu BASE, BASE, 8
>>     |  // Remember active hook before pcall.
>>     |  srl TMP3, TMP3, HOOK_ACTIVE_SHIFT
>> @@ -1394,8 +1398,12 @@ static void build_subroutines(BuildCtx *ctx)
>>     |.  addiuNARGS8:RC,NARGS8:RC, -8
>>     |
>>     |.ffunc xpcall
>> +  |   lw TMP1, L->maxstack
>> +  |   addu TMP2, BASE,NARGS8:RC
>>     |    sltiu AT,NARGS8:RC, 16
>>     |  lw CARG4, 8+HI(BASE)
>> +  |   sltu TMP1, TMP1, TMP2
>> +  |    or AT, AT, TMP1
>>     |    bnez AT, ->fff_fallback
>>     |.  lw CARG3, 8+LO(BASE)
>>     |   lw CARG1, LO(BASE)
>> diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
>> index 7f49df5b..06b143a2 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
> I see that the original patch in the upstream has another diff. Please
> backport the commit ea7071d3 ("MIPS64: Fix xpcall() error case") first
> (as the first commit in the patch series) to avoid conflicts in the
> future.\
Done.
>
>> @@ -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)
>>     |  daddiuNARGS8:RC,NARGS8:RC, -16
>> -  |  ld CARG1, 0(BASE)
>>     |   ld CARG2, 8(BASE)
>>     |    bltzNARGS8:RC, ->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
>> @@ -1755,8 +1755,12 @@ static void build_subroutines(BuildCtx *ctx)
>>     |//-- Base library: catch errors ----------------------------------------
>>     |
>>     |.ffunc pcall
>> +  |    lwz TMP1, L->maxstack
>> +  |    add TMP2, BASE,NARGS8:RC
>>     |  cmplwiNARGS8:RC, 8
>>     |   lbz TMP3, DISPATCH_GL(hookmask)(DISPATCH)
>> +  |    cmplw cr1, TMP1, TMP2
>> +  |  cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt
>>     |  blt ->fff_fallback
>>     |   mr TMP2, BASE
>>     |   la BASE, 8(BASE)
>> @@ -1767,14 +1771,19 @@ static void build_subroutines(BuildCtx *ctx)
>>     |  b ->vm_call_dispatch
>>     |
>>     |.ffunc xpcall
>> +  |     lwz TMP1, L->maxstack
>> +  |     add TMP2, BASE,NARGS8:RC
>>     |  cmplwiNARGS8:RC, 16
>>     |   lwz CARG3, 8(BASE)
>> +  |     cmplw cr1, TMP1, TMP2
>>     |.if FPU
>>     |    lfd FARG2, 8(BASE)
>> +  |  cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt
>>     |    lfd FARG1, 0(BASE)
>>     |.else
>>     |    lwz CARG1, 0(BASE)
>>     |    lwz CARG2, 4(BASE)
>> +  |  cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt
>>     |    lwz CARG4, 12(BASE)
>>     |.endif
>>     |  blt ->fff_fallback
>> 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
>> @@ -1545,6 +1545,9 @@ static void build_subroutines(BuildCtx *ctx)
>>     |//-- Base library: catch errors ----------------------------------------
>>     |
>>     |.ffunc_1 pcall
>> +  |  movL:RB, SAVE_L
>> +  |  lea RA, [BASE+NARGS:RD*8]
>> +  |  cmp RA,L:RB->maxstack; ja ->fff_fallback
>>     |  lea RA, [BASE+16]
>>     |  subNARGS:RDd, 1
>>     |  mov PCd, 16+FRAME_PCALL
>> @@ -1563,6 +1566,9 @@ static void build_subroutines(BuildCtx *ctx)
>>     |  jmp ->vm_call_dispatch
>>     |
>>     |.ffunc_2 xpcall
>> +  |  movL:RB, SAVE_L
>> +  |  lea RA, [BASE+NARGS:RD*8]
>> +  |  cmp RA,L:RB->maxstack; ja ->fff_fallback
>>     |  movLFUNC:RA, [BASE+8]
>>     |  checktp_ncLFUNC:RA, LJ_TFUNC, ->fff_fallback
>>     |  movLFUNC:RB, [BASE]		// Swap function and traceback.
>> 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
>> @@ -1914,6 +1914,9 @@ static void build_subroutines(BuildCtx *ctx)
>>     |//-- Base library: catch errors ----------------------------------------
>>     |
>>     |.ffunc_1 pcall
>> +  |  movL:RB, SAVE_L
>> +  |  lea RA, [BASE+NARGS:RD*8]
>> +  |  cmp RA,L:RB->maxstack; ja ->fff_fallback
>>     |  lea RA, [BASE+8]
>>     |  subNARGS:RD, 1
>>     |  mov PC, 8+FRAME_PCALL
>> @@ -1925,6 +1928,9 @@ static void build_subroutines(BuildCtx *ctx)
>>     |  jmp ->vm_call_dispatch
>>     |
>>     |.ffunc_2 xpcall
>> +  |  movL:RB, SAVE_L
>> +  |  lea RA, [BASE+NARGS:RD*8]
>> +  |  cmp RA,L:RB->maxstack; ja ->fff_fallback
>>     |  cmp dword [BASE+12], LJ_TFUNC;  jne ->fff_fallback
>>     |  mov RB, [BASE+4]			// Swap function and traceback.
>>     |  mov [BASE+12], RB
>> 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 e300d5c1..367aecb6 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
>> @@ -7,7 +7,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)
>>   
>>   -- The first testcase demonstrate a stack overflow in `pcall()`
>>   -- by recursive calling `pcall()`. The functions are vararg
>> @@ -53,4 +53,33 @@ pcall(coroutine.wrap(looper_2), 0)
>>   
>>   test:ok(true, 'no stack overflow with using metamethod')
>>   
> Why do you drop the original test case?
>
> Expected behaviour:
> | src/luajit -e 'local t = {setmetatable({},{__call=pcall})()} print(t[#t])'
> | stack overflow
> Actual behaviour -- dirty read detected by ASAN.

this testcase is failed too:

TAP version 13
1..4
ok - no stack overflow with recursive pcall
ok - no stack overflow with metamethod
=================================================================
==3374==ERROR: AddressSanitizer: heap-buffer-overflow on address 
0x007f77c03fc8 at pc 0x005574fc4fb0 bp 0x007ff2e0f3f0
  sp 0x007ff2e0f410
WRITE of size 8 at 0x007f77c03fc8 thread T0
     #0 0x5574fc4faf in copyTV /root/sergeyb/luajit/src/lj_obj.h:1000
     #1 0x5574fc4faf in lj_meta_call /root/sergeyb/luajit/src/lj_meta.c:444
     #2 0x557510c293 in lj_vmeta_call 
(/root/sergeyb/luajit/build/src/luajit+0x1d5293)
     #3 0x5574f92a9b in lua_pcall /root/sergeyb/luajit/src/lj_api.c:1173
     #4 0x5574f74217 in docall /root/sergeyb/luajit/src/luajit.c:134
     #5 0x5574f75067 in handle_script /root/sergeyb/luajit/src/luajit.c:304
     #6 0x5574f76d07 in pmain /root/sergeyb/luajit/src/luajit.c:602
     #7 0x557510bb33 in lj_BC_FUNCC 
(/root/sergeyb/luajit/build/src/luajit+0x1d4b33)
     #8 0x5574f937fb in lua_cpcall /root/sergeyb/luajit/src/lj_api.c:1208
     #9 0x5574f77083 in main /root/sergeyb/luajit/src/luajit.c:633
     #10 0x7f7dbfe79f in __libc_start_main 
(/lib/aarch64-linux-gnu/libc.so.6+0x2079f)
     #11 0x5574f73aa3 (/root/sergeyb/luajit/build/src/luajit+0x3caa3)

>
>> +-- The third testcase demonstrate a stack overflow in
>> +-- `pcall()`/xpcall()` similar to the first testcase, but it is
>> +-- triggered using hand-crafted Lua chunk with a lot `pcall()`
>> +-- builtins.
>> +
>> +for i = 1, 100 do
>> +  local code = 'return pcall(' .. string.rep('pcall, ', i) .. 'pairs, {})'
> Why do we need this test if it has the same semantics as the second
> one?
the test was removed
>
>> +  local f = load(code)
>> +  coroutine.wrap(f)()
>> +end
>> +
>> +test:ok(true, 'no stack overflow with pcalls in load()')
>> +
>> +-- The fourth testcase demonstrate a stack overflow in
>> +-- `pcall()`/`xpcall()` similar to the first testcase, but it is
>> +-- triggered using `unpack()`.
>> +
>> +local t = {}
>> +local function f()
>> +  return pcall(unpack(t))
>> +end
>> +
>> +for i = 1, 100 do
> This limit isn't enough for GC64 or non-GC64 mode.
> | src/luajit -e '
> | local t = {}
> | local function f() return pcall(unpack(t)) end
> | for i = 1, 100 do
> |   t[i], t[i+1], t[i+2] = pcall, pairs, {}
> |   coroutine.wrap(f)()
> | end
> | '
>
> For the GC64 build it is necessary to set the limit as 180, (179 -- not
> SegFault).
>
> Please provide two different limits depending on the GC64 mode
> configuration. Please, describe why the __exact__ limit is chosen for
> the particular configuration.
Added.
>
>> +  t[i], t[i + 1], t[i + 2] = pcall, pairs, {}
>> +  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: 19135 bytes --]