Skip to content

Commit ca1c72e

Browse files
Mike Pallligurio
authored andcommitted
Add stack check to pcall/xpcall.
Analyzed by Peter Cawley. (cherry picked from commit a4c1640) In the commit "LJ_FR2: Fix stack checks in vararg calls." stack overflow in `pcall()`/`xpcall()` was fixed partially and there are still cases where stack overflow happens, see comments in the test. The patch add stack check to `pcall()` and `xpcall()`. Sergey Bronnikov: * added the description and the test for the problem Part of tarantool/tarantool#11691
1 parent 75cdb72 commit ca1c72e

File tree

8 files changed

+86
-5
lines changed

8 files changed

+86
-5
lines changed

src/vm_arm.dasc

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1201,8 +1201,11 @@ static void build_subroutines(BuildCtx *ctx)
12011201
|//-- Base library: catch errors ----------------------------------------
12021202
|
12031203
|.ffunc pcall
1204+
| ldr RB, L->maxstack
1205+
| add INS, BASE, NARGS8:RC
12041206
| ldrb RA, [DISPATCH, #DISPATCH_GL(hookmask)]
12051207
| cmp NARGS8:RC, #8
1208+
| cmphs RB, INS
12061209
| blo ->fff_fallback
12071210
| tst RA, #HOOK_ACTIVE // Remember active hook before pcall.
12081211
| mov RB, BASE
@@ -1213,7 +1216,11 @@ static void build_subroutines(BuildCtx *ctx)
12131216
| b ->vm_call_dispatch
12141217
|
12151218
|.ffunc_2 xpcall
1219+
| ldr RB, L->maxstack
1220+
| add INS, BASE, NARGS8:RC
12161221
| ldrb RA, [DISPATCH, #DISPATCH_GL(hookmask)]
1222+
| cmp RB, INS
1223+
| blo ->fff_fallback
12171224
| checkfunc CARG4, ->fff_fallback // Traceback must be a function.
12181225
| mov RB, BASE
12191226
| strd CARG12, [BASE, #8] // Swap function and traceback.

src/vm_arm64.dasc

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1166,6 +1166,10 @@ static void build_subroutines(BuildCtx *ctx)
11661166
|//-- Base library: catch errors ----------------------------------------
11671167
|
11681168
|.ffunc pcall
1169+
| ldr TMP1, L->maxstack
1170+
| add TMP2, BASE, NARGS8:RC
1171+
| cmp TMP1, TMP2
1172+
| blo ->fff_fallback
11691173
| cmp NARGS8:RC, #8
11701174
| ldrb TMP0w, GL->hookmask
11711175
| blo ->fff_fallback
@@ -1185,6 +1189,10 @@ static void build_subroutines(BuildCtx *ctx)
11851189
| b ->vm_call_dispatch
11861190
|
11871191
|.ffunc xpcall
1192+
| ldr TMP1, L->maxstack
1193+
| add TMP2, BASE, NARGS8:RC
1194+
| cmp TMP1, TMP2
1195+
| blo ->fff_fallback
11881196
| ldp CARG1, CARG2, [BASE]
11891197
| ldrb TMP0w, GL->hookmask
11901198
| subs NARGS8:TMP1, NARGS8:RC, #16

src/vm_mips.dasc

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1382,9 +1382,13 @@ static void build_subroutines(BuildCtx *ctx)
13821382
|//-- Base library: catch errors ----------------------------------------
13831383
|
13841384
|.ffunc pcall
1385+
| lw TMP1, L->maxstack
1386+
| addu TMP2, BASE, NARGS8:RC
13851387
| lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
13861388
| beqz NARGS8:RC, ->fff_fallback
1387-
| move TMP2, BASE
1389+
|. sltu AT, TMP1, TMP2
1390+
| bnez AT, ->fff_fallback
1391+
|. move TMP2, BASE
13881392
| addiu BASE, BASE, 8
13891393
| // Remember active hook before pcall.
13901394
| srl TMP3, TMP3, HOOK_ACTIVE_SHIFT
@@ -1394,8 +1398,12 @@ static void build_subroutines(BuildCtx *ctx)
13941398
|. addiu NARGS8:RC, NARGS8:RC, -8
13951399
|
13961400
|.ffunc xpcall
1401+
| lw TMP1, L->maxstack
1402+
| addu TMP2, BASE, NARGS8:RC
13971403
| sltiu AT, NARGS8:RC, 16
13981404
| lw CARG4, 8+HI(BASE)
1405+
| sltu TMP1, TMP1, TMP2
1406+
| or AT, AT, TMP1
13991407
| bnez AT, ->fff_fallback
14001408
|. lw CARG3, 8+LO(BASE)
14011409
| lw CARG1, LO(BASE)

src/vm_mips64.dasc

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1418,8 +1418,12 @@ static void build_subroutines(BuildCtx *ctx)
14181418
|//-- Base library: catch errors ----------------------------------------
14191419
|
14201420
|.ffunc pcall
1421+
| ld TMP1, L->maxstack
1422+
| daddu TMP2, BASE, NARGS8:RC
1423+
| sltu AT, TMP1, TMP2
1424+
| bnez AT, ->fff_fallback
1425+
|. lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
14211426
| daddiu NARGS8:RC, NARGS8:RC, -8
1422-
| lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH)
14231427
| bltz NARGS8:RC, ->fff_fallback
14241428
|. move TMP2, BASE
14251429
| daddiu BASE, BASE, 16
@@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx)
14401444
|. nop
14411445
|
14421446
|.ffunc xpcall
1443-
| daddiu NARGS8:TMP0, NARGS8:RC, -16
1444-
| ld CARG1, 0(BASE)
1447+
| ld TMP1, L->maxstack
1448+
| daddu TMP2, BASE, NARGS8:RC
1449+
| sltu AT, TMP1, TMP2
1450+
| bnez AT, ->fff_fallback
1451+
|. ld CARG1, 0(BASE)
1452+
| daddiu NARGS8:RC, NARGS8:RC, -16
14451453
| ld CARG2, 8(BASE)
14461454
| bltz NARGS8:TMP0, ->fff_fallback
14471455
|. lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH)

src/vm_ppc.dasc

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1755,8 +1755,12 @@ static void build_subroutines(BuildCtx *ctx)
17551755
|//-- Base library: catch errors ----------------------------------------
17561756
|
17571757
|.ffunc pcall
1758+
| lwz TMP1, L->maxstack
1759+
| add TMP2, BASE, NARGS8:RC
17581760
| cmplwi NARGS8:RC, 8
17591761
| lbz TMP3, DISPATCH_GL(hookmask)(DISPATCH)
1762+
| cmplw cr1, TMP1, TMP2
1763+
| cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt
17601764
| blt ->fff_fallback
17611765
| mr TMP2, BASE
17621766
| la BASE, 8(BASE)
@@ -1767,14 +1771,19 @@ static void build_subroutines(BuildCtx *ctx)
17671771
| b ->vm_call_dispatch
17681772
|
17691773
|.ffunc xpcall
1774+
| lwz TMP1, L->maxstack
1775+
| add TMP2, BASE, NARGS8:RC
17701776
| cmplwi NARGS8:RC, 16
17711777
| lwz CARG3, 8(BASE)
1778+
| cmplw cr1, TMP1, TMP2
17721779
|.if FPU
17731780
| lfd FARG2, 8(BASE)
1781+
| cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt
17741782
| lfd FARG1, 0(BASE)
17751783
|.else
17761784
| lwz CARG1, 0(BASE)
17771785
| lwz CARG2, 4(BASE)
1786+
| cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt
17781787
| lwz CARG4, 12(BASE)
17791788
|.endif
17801789
| blt ->fff_fallback

src/vm_x64.dasc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1545,6 +1545,9 @@ static void build_subroutines(BuildCtx *ctx)
15451545
|//-- Base library: catch errors ----------------------------------------
15461546
|
15471547
|.ffunc_1 pcall
1548+
| mov L:RB, SAVE_L
1549+
| lea RA, [BASE+NARGS:RD*8]
1550+
| cmp RA, L:RB->maxstack; ja ->fff_fallback
15481551
| lea RA, [BASE+16]
15491552
| sub NARGS:RDd, 1
15501553
| mov PCd, 16+FRAME_PCALL
@@ -1563,6 +1566,9 @@ static void build_subroutines(BuildCtx *ctx)
15631566
| jmp ->vm_call_dispatch
15641567
|
15651568
|.ffunc_2 xpcall
1569+
| mov L:RB, SAVE_L
1570+
| lea RA, [BASE+NARGS:RD*8]
1571+
| cmp RA, L:RB->maxstack; ja ->fff_fallback
15661572
| mov LFUNC:RA, [BASE+8]
15671573
| checktp_nc LFUNC:RA, LJ_TFUNC, ->fff_fallback
15681574
| mov LFUNC:RB, [BASE] // Swap function and traceback.

src/vm_x86.dasc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1914,6 +1914,9 @@ static void build_subroutines(BuildCtx *ctx)
19141914
|//-- Base library: catch errors ----------------------------------------
19151915
|
19161916
|.ffunc_1 pcall
1917+
| mov L:RB, SAVE_L
1918+
| lea RA, [BASE+NARGS:RD*8]
1919+
| cmp RA, L:RB->maxstack; ja ->fff_fallback
19171920
| lea RA, [BASE+8]
19181921
| sub NARGS:RD, 1
19191922
| mov PC, 8+FRAME_PCALL
@@ -1925,6 +1928,9 @@ static void build_subroutines(BuildCtx *ctx)
19251928
| jmp ->vm_call_dispatch
19261929
|
19271930
|.ffunc_2 xpcall
1931+
| mov L:RB, SAVE_L
1932+
| lea RA, [BASE+NARGS:RD*8]
1933+
| cmp RA, L:RB->maxstack; ja ->fff_fallback
19281934
| cmp dword [BASE+12], LJ_TFUNC; jne ->fff_fallback
19291935
| mov RB, [BASE+4] // Swap function and traceback.
19301936
| mov [BASE+12], RB

test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({
77
['Test requires JIT enabled'] = not jit.status(),
88
})
99

10-
test:plan(2)
10+
test:plan(4)
1111

1212
-- The first testcase demonstrate a stack overflow in `pcall()`
1313
-- by recursive calling `pcall()`. The functions are vararg
@@ -53,4 +53,33 @@ pcall(coroutine.wrap(looper_2), 0)
5353

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

56+
-- The third testcase demonstrate a stack overflow in
57+
-- `pcall()`/xpcall()` similar to the first testcase, but it is
58+
-- triggered using hand-crafted Lua chunk with a lot `pcall()`
59+
-- builtins.
60+
61+
for i = 1, 100 do
62+
local code = 'return pcall(' .. string.rep('pcall, ', i) .. 'pairs, {})'
63+
local f = load(code)
64+
coroutine.wrap(f)()
65+
end
66+
67+
test:ok(true, 'no stack overflow with pcalls in load()')
68+
69+
-- The fourth testcase demonstrate a stack overflow in
70+
-- `pcall()`/`xpcall()` similar to the first testcase, but it is
71+
-- triggered using `unpack()`.
72+
73+
local t = {}
74+
local function f()
75+
return pcall(unpack(t))
76+
end
77+
78+
for i = 1, 100 do
79+
t[i], t[i + 1], t[i + 2] = pcall, pairs, {}
80+
coroutine.wrap(f)()
81+
end
82+
83+
test:ok(true, 'no stack overflow with unpacked pcalls')
84+
5685
test:done(true)

0 commit comments

Comments
 (0)