Hi, Sergey,
thanks for review! See my comments below.
Sergey
UpdatedHi, 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.
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(-) 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 5ef37243..074c1f31 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) | daddiu NARGS8:RC, NARGS8:RC, -8 - | lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH) | bltz NARGS8:RC, ->fff_fallback |. move TMP2, BASE | daddiu BASE, BASE, 16 @@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx) |. nop | |.ffunc xpcall - | daddiu NARGS8:TMP0, NARGS8:RC, -16This 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)+ | daddiu NARGS8:RC, NARGS8:RC, -16This line is incorrect. This neglets the 1st patch in the series. It should be | | daddiu NARGS8:TMP0, NARGS8:RC, -16
Right. However, probably we should leave this line near ".ffunc xpcall". What do you think?
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)
- | 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 CARG2, 8(BASE) | bltz NARGS8: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 8b6781a6..c57b76b7 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 7c11c78e..36804d11 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 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') -- See also https://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
+
+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')
@@ -66,7 +68,7 @@ test:ok(true, 'no stack overflow with metamethod __call with 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 +-- triggered using `unpack()`. + +t = {} +local function f()Lets name it `pcall_wrapper()`
return pcall(unpack(t))+ return pcall(unpack(t)) +end + +-- The problem is only reproduced on LuaJIT GC64 and is bestTypo: s/best/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 do + t[i], t[i + 1], t[i + 2] = pcall, type, {} + coroutine.wrap(f)() +end + +test:ok(true, 'no stack overflow with unpacked pcalls') + test:done(true) -- 2.43.0