Hi, Sergey,

I've updated N_ITERATIONS and force-pushed the branch.

Sergey

On 3/12/26 20:19, Sergey Kaplun wrote:

Hi, Sergey!
Thanks for the fixes!

LGTM, after fixing the last nit below.

On 12.03.26, Sergey Bronnikov wrote:

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(-)

<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?

Why? This break the `maxstack` check (since the RC is differs before the
addition with TMP2).

Ok, let's leave as is, seems I took a look on intermediate version, before applying changes you requested on review.

The current patch looks fine for me:


See the latest LuaJIT version:
https://github.com/LuaJIT/LuaJIT/blob/659a61693aa3b87661864ad0f12eee14c865cd7f/src/vm_mips64.dasc#L1450

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)

<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

Why do we need two variables with the same value of iterations?
Let's use N_ITERATIONS with the comment for xpcall and pcall.

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 825568f9..036d53e9 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
@@ -63,9 +63,13 @@ local function xpcall_wrapper()
return xpcall(unpack(t))
end

-local N_ITERATIONS_1 = 200
+-- The problems are 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

-for i = 1, N_ITERATIONS_1 do
+for i = 1, N_ITERATIONS do
t[i], t[i + 1], t[i + 2] = xpcall, type, {}
coroutine.wrap(xpcall_wrapper)()
end
@@ -81,13 +85,7 @@ local function pcall_wrapper()
return pcall(unpack(t))
end

--- 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_2 = 200
-
-for i = 1, N_ITERATIONS_2 do
+for i = 1, N_ITERATIONS do
t[i], t[i + 1], t[i + 2] = pcall, type, {}
coroutine.wrap(pcall_wrapper)()
end

+
+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()

<snipped>

-- 
2.43.0