Hi, Sergey,

thanks for review!

Sergey

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 10.12.25, Sergey Bronnikov wrote:

From: Mike Pall <mike>

Analyzed by Peter Cawley.

(cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)

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

Please describe the issue regardless previous commit. Just mentioned
missing stack checks for `pcall()`, `xpcall()` is enough.

Updated commit message:

    Add stack check to pcall/xpcall.
    
    Analyzed by Peter Cawley.
    
    (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36)
    
    The patch adds 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

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
---
 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 | 31 ++++++++++++++++++-
 8 files changed, 86 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 cf8e575a..53ff7162 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, -16

This change is incorrect.
It wipes out the first patch in the series.

Sorry, my bad.

--- 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 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, -16
   |   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 d5296759..141f5f82 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 b043b830..1ba5abce 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 d471d41e..b135042b 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
@@ -1,4 +1,5 @@
 local tap = require('tap')
+local ffi = require('ffi')
 
 -- A test file to demonstrate a stack overflow in `pcall()` in
 -- some cases, see below testcase descriptions.
@@ -7,7 +8,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)

This patch covers only `pcall()` cases, please add the same tests for
`xpcall()` (I suppose the most simple is `xpcall()` as `__call`
metamethod).

Added:

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 0ce3c756..8f45941c 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
@@ -6,7 +6,7 @@ local ffi = require('ffi')
 -- See also https://github.com/LuaJIT/LuaJIT/issues/1048.
 local test = tap.test('lj-1048-fix-stack-checks-vararg-calls')
 
-test:plan(4)
+test:plan(5)
 
 -- The test case demonstrates a segmentation fault due to stack
 -- overflow by recursive calling `pcall()`. The functions are
@@ -51,12 +51,16 @@ pcall(coroutine.wrap(looper), prober_2, 0)
 
 test:ok(true, 'no stack overflow with metamethod')
 
--- The testcase demonstrates a stack overflow in
+-- The testcases demonstrates a stack overflow in
 -- `pcall()`/xpcall()` triggered using metamethod `__call`.
 
 t = coroutine.wrap(setmetatable)({}, { __call = pcall })
 
-test:ok(true, 'no stack overflow with metamethod __call')
+test:ok(true, 'no stack overflow with metamethod __call with pcall()')
+
+t = coroutine.wrap(setmetatable)({}, { __call = 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

 
 -- The testcase demonstrate a segmentation fault due to stack
 -- overflow by recursive calling `pcall()`. The functions are
@@ -50,4 +51,32 @@ pcall(coroutine.wrap(looper), prober_2, 0)
 
 test:ok(true, 'no stack overflow with metamethod')
 
+-- The testcase demonstrate a stack overflow in

Typo: s/demonstrate/demonstrates/

+-- `pcall()`/xpcall()` triggered using metamethod `__call`.
+
+t = setmetatable({}, { __call = pcall })()

It's better to do this at the separate Lua stack, i.e. inside
`coroutine.wrap()`.

Updated.

 -- `pcall()`/xpcall()` triggered using metamethod `__call`.
 
-t = setmetatable({}, { __call = pcall })()
+t = coroutine.wrap(setmetatable)({}, { __call = pcall })

+
+test:ok(true, 'no stack overflow with metamethod __call')
+
+-- The testcase demonstrate a stack overflow in

Typo: s/demonstrate/demonstrates/

+-- `pcall()`/`xpcall()` similar to the first testcase, but it is
+-- triggered using `unpack()`.
+
+t = {}
+local function f()
+  return pcall(unpack(t))
+end
+

Please explain the amount of necessary iterations of calls.

After a small research we found that issue cannot be reproduced in LuaJIT GC32 flavors.

N_ITERATION should be equal to at least 200 for reproducing.

The problem reproduced better under Valgrind than ASAN.

+local N_ITERATIONS = 100
+if ffi.abi('gc64') then
+  N_ITERATIONS = 180
+end
+
+for i = 1, N_ITERATIONS do
+  t[i], t[i + 1], t[i + 2] = pcall, pairs, {}

Let's use `type` here instead of pairs.

+  coroutine.wrap(f)()
+end
+
+test:ok(true, 'no stack overflow with unpacked pcalls')
+
 test:done(true)
-- 
2.43.0