[Tarantool-patches] [PATCH luajit 3/3][v3] Add stack check to pcall/xpcall.
Sergey Bronnikov
sergeyb at tarantool.org
Thu Mar 12 19:23:20 MSK 2026
Hi, Sergey,
thanks for review! See my comments below.
Sergey
On 3/12/26 13:16, Sergey Kaplun via Tarantool-patches wrote:
> Hi, 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.
Updated
>> 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)
>> | 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 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)
>> + | daddiuNARGS8:RC,NARGS8:RC, -16
> This line is incorrect. This neglets the 1st patch in the series.
>
> It should be
> | | daddiuNARGS8: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)
>> | 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 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 alsohttps://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')
>> +
>> +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()`
>
@@ -66,7 +68,7 @@ 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
@@ -78,7 +80,7 @@ local N_ITERATIONS = 200
for i = 1, N_ITERATIONS 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')
>> + return pcall(unpack(t))
>> +end
>> +
>> +-- The problem is only reproduced on LuaJIT GC64 and is best
> Typo: s/best/better/
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.
>
>> +-- 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
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20260312/6f68d32f/attachment.htm>
More information about the Tarantool-patches
mailing list