Skip to content

Commit 4a85287

Browse files
Mike Pallligurio
authored andcommitted
Add stack check to pcall/xpcall.
Analyzed by Peter Cawley. (cherry picked from commit a4c1640) 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()`. Sergey Bronnikov: * added the description and the test for the problem Part of tarantool/tarantool#12134
1 parent 51e75e7 commit 4a85287

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
@@ -1,4 +1,5 @@
11
local tap = require('tap')
2+
local ffi = require('ffi')
23

34
-- A test file to demonstrate a stack overflow in `pcall()` in
45
-- some cases, see below testcase descriptions.
@@ -7,7 +8,7 @@ local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({
78
['Test requires JIT enabled'] = not jit.status(),
89
})
910

10-
test:plan(2)
11+
test:plan(4)
1112

1213
-- The testcase demonstrate a segmentation fault due to stack
1314
-- overflow by recursive calling `pcall()`. The functions are
@@ -50,4 +51,32 @@ pcall(coroutine.wrap(looper), prober_2, 0)
5051

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

54+
-- The testcase demonstrate a stack overflow in
55+
-- `pcall()`/xpcall()` triggered using metamethod `__call`.
56+
57+
t = setmetatable({}, { __call = pcall })()
58+
59+
test:ok(true, 'no stack overflow with metamethod __call')
60+
61+
-- The testcase demonstrate a stack overflow in
62+
-- `pcall()`/`xpcall()` similar to the first testcase, but it is
63+
-- triggered using `unpack()`.
64+
65+
t = {}
66+
local function f()
67+
return pcall(unpack(t))
68+
end
69+
70+
local N_ITERATIONS = 100
71+
if ffi.abi('gc64') then
72+
N_ITERATIONS = 180
73+
end
74+
75+
for i = 1, N_ITERATIONS do
76+
t[i], t[i + 1], t[i + 2] = pcall, pairs, {}
77+
coroutine.wrap(f)()
78+
end
79+
80+
test:ok(true, 'no stack overflow with unpacked pcalls')
81+
5382
test:done(true)

0 commit comments

Comments
 (0)