[Tarantool-patches] [PATCH luajit v4 0/3] Improve error reporting on stack overflow

[Tarantool-patches] [PATCH luajit v4 0/3] Improve error reporting on stack overflow

[Maxim Kokryashkin via Tarantool-patches]

Changes in v4:
- Added skipcond for nounwind builds.

Issues: https://github.com/luajit/luajit/issues/962
https://github.com/tarantool/tarantool/issues/9145

PR: https://github.com/tarantool/tarantool/pull/9380
Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-962-error-reporting-on-stack-overflow

Mike Pall (3):
  Improve error reporting on stack overflow.
  Cleanup stack overflow handling.
  Follow-up fix for stack overflow handling cleanup.

 src/lj_state.c                                | 15 ++-
 .../lj-962-premature-stack-overflow.test.c    | 95 +++++++++++++++++++
 .../lj-603-err-snap-restore.test.lua          |  1 +
 .../lj-962-stack-overflow-report.test.lua     | 10 ++
 .../lj-962-stack-overflow-report/script.lua   |  3 +
 test/tarantool-tests/utils/exec.lua           | 15 ++-
 6 files changed, 132 insertions(+), 7 deletions(-)
 create mode 100644 test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
 create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report.test.lua
 create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report/script.lua

--
2.43.0

[Tarantool-patches] [PATCH luajit v4 1/3] Improve error reporting on stack overflow.

[Maxim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Nicolas Lebedenco.

(cherry-picked from commit 8135de2a0204e6acd92b231131c4a1e0be03ac1c)

The `lj_state_growstack` doesn't account for a potential error
handler invocation by `xpcall`, which may lead to the second
error while handling a stack overflow, resulting in a misleading
error "error in error handling", while the real issue is a stack
overflow. This patch addresses this issue by fixing the condition
at which stack overflow errors are thrown. Now it's thrown if the
stack size is at least at the limit, instead of when it is over
the limit.

This commit also disables the second test from
`lj-603-err-snap-restore`, since after this patch and the two
follow-ups for it, there is no such amount of stack slots with
which the test works the way it should.

Lastly, this patch adds an alternative to `luacmd` to the
`utils.exec` module, which is called `luabin`. That function is
similar to the `luacmd`, with the only difference of returning
only the executable itself without any additional CLI options
passed.

Maxim Kokryashkin:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
 src/lj_state.c                                    |  2 +-
 .../lj-603-err-snap-restore.test.lua              |  1 +
 .../lj-962-stack-overflow-report.test.lua         | 10 ++++++++++
 .../lj-962-stack-overflow-report/script.lua       |  3 +++
 test/tarantool-tests/utils/exec.lua               | 15 ++++++++++++---
 5 files changed, 27 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report.test.lua
 create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report/script.lua

diff --git a/src/lj_state.c b/src/lj_state.c
index 684336d5..76153bad 100644
--- a/src/lj_state.c
+++ b/src/lj_state.c
@@ -132,7 +132,7 @@ void LJ_FASTCALL lj_state_growstack(lua_State *L, MSize need)
       n = LJ_STACK_MAX;
   }
   resizestack(L, n);
-  if (L->stacksize > LJ_STACK_MAXEX)
+  if (L->stacksize >= LJ_STACK_MAXEX)
     lj_err_msg(L, LJ_ERR_STKOV);
 }
 
diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
index 96ebf92c..f5c8474f 100644
--- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
+++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
@@ -36,6 +36,7 @@ local function do_test()
     -- Tarantool at start, so just skip test for it.
     -- luacheck: no global
     ['Disable test for Tarantool'] = _TARANTOOL,
+    ['Stack overflow is now handled differently'] = true,
   })
 
   test:ok(not handler_is_called)
diff --git a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
new file mode 100644
index 00000000..45a888f4
--- /dev/null
+++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
@@ -0,0 +1,10 @@
+local tap = require('tap')
+-- The test reproduces the problem only for GC64 mode with enabled JIT.
+local test = tap.test('lj-962-stack-overflow-report')
+test:plan(1)
+
+local LUABIN = require('utils').exec.luabin(arg)
+local SCRIPT = arg[0]:gsub('%.test%.lua$', '/script.lua')
+local output = io.popen(LUABIN .. ' 2>&1 ' .. SCRIPT, 'r'):read('*all')
+test:like(output, 'stack overflow', 'stack overflow reported correctly')
+test:done(true)
diff --git a/test/tarantool-tests/lj-962-stack-overflow-report/script.lua b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
new file mode 100644
index 00000000..31c5ca33
--- /dev/null
+++ b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
@@ -0,0 +1,3 @@
+-- XXX: Function `f` is global to avoid using an additional stack slot.
+-- luacheck: no global
+f = function() f() end; f()
diff --git a/test/tarantool-tests/utils/exec.lua b/test/tarantool-tests/utils/exec.lua
index a56ca2dc..48a08ba5 100644
--- a/test/tarantool-tests/utils/exec.lua
+++ b/test/tarantool-tests/utils/exec.lua
@@ -1,14 +1,23 @@
 local M = {}
 
-function M.luacmd(args)
+local function executable_idx(args)
   -- arg[-1] is guaranteed to be not nil.
   local idx = -2
   while args[idx] do
     assert(type(args[idx]) == 'string', 'Command part have to be a string')
     idx = idx - 1
   end
-  -- return the full command with flags.
-  return table.concat(args, ' ', idx + 1, -1)
+  return idx + 1
+end
+
+function M.luabin(args)
+  -- Return only the executable.
+  return args[executable_idx(args)]
+end
+
+function M.luacmd(args)
+  -- Return the full command with flags.
+  return table.concat(args, ' ', executable_idx(args), -1)
 end
 
 local function makeenv(tabenv)
-- 
2.43.0

[Tarantool-patches] [PATCH luajit v4 2/3] Cleanup stack overflow handling.

[Maxim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

Reported by Peter Cawley.

(cherry-picked from commit d2f6c55b05c716e5dbb479b7e684abaee7cf6e12)

After the previous patch, it is possible to trigger the
`stack overflow` error prematurely. Consider the following
situation: there are already 33000 slots allocated on a Lua
stack, and then there are 30 additional slots needed. In this
case, the actual allocated amount would be twice the already
allocated size, shrunk to the `LJ_STACK_MAXEX` size, which
would lead to the stack overflow error, despite the fact there
is plenty of unused space. This patch completely reworks the
logic of error handling during stack growth to address the issue.

Another important thing to notice is that the `LJ_ERR_STKOV` is
thrown only if the `L->status` is `LUA_OK` and that status is set
to `LUA_ERRRUN` just before throwing the error. The status is set
to `LUA_ERRRUN` to avoid the second stack overflow during the
`err_msgv` execution.

Maxim Kokryashkin:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
 src/lj_state.c                                | 15 +++--
 .../lj-962-premature-stack-overflow.test.c    | 63 +++++++++++++++++++
 2 files changed, 74 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c

diff --git a/src/lj_state.c b/src/lj_state.c
index 76153bad..d8a5134c 100644
--- a/src/lj_state.c
+++ b/src/lj_state.c
@@ -121,8 +121,17 @@ void lj_state_shrinkstack(lua_State *L, MSize used)
 void LJ_FASTCALL lj_state_growstack(lua_State *L, MSize need)
 {
   MSize n;
-  if (L->stacksize > LJ_STACK_MAXEX)  /* Overflow while handling overflow? */
-    lj_err_throw(L, LUA_ERRERR);
+  if (L->stacksize >= LJ_STACK_MAXEX) {
+    /* 4. Throw 'error in error handling' when we are _over_ the limit. */
+    if (L->stacksize > LJ_STACK_MAXEX)
+      lj_err_throw(L, LUA_ERRERR);  /* Does not invoke an error handler. */
+    /* 1. We are _at_ the limit after the last growth. */
+    if (!L->status) {  /* 2. Throw 'stack overflow'. */
+      L->status = LUA_ERRRUN;  /* Prevent ending here again for pushed msg. */
+      lj_err_msg(L, LJ_ERR_STKOV);  /* May invoke an error handler. */
+    }
+    /* 3. Add space (over the limit) for pushed message and error handler. */
+  }
   n = L->stacksize + need;
   if (n > LJ_STACK_MAX) {
     n += 2*LUA_MINSTACK;
@@ -132,8 +141,6 @@ void LJ_FASTCALL lj_state_growstack(lua_State *L, MSize need)
       n = LJ_STACK_MAX;
   }
   resizestack(L, n);
-  if (L->stacksize >= LJ_STACK_MAXEX)
-    lj_err_msg(L, LJ_ERR_STKOV);
 }
 
 void LJ_FASTCALL lj_state_growstack1(lua_State *L)
diff --git a/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c b/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
new file mode 100644
index 00000000..12cb9004
--- /dev/null
+++ b/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
@@ -0,0 +1,63 @@
+#include "lua.h"
+#include "lauxlib.h"
+
+#include "test.h"
+#include "utils.h"
+
+/*
+ * XXX: The "lj_obj.h" header is included to calculate the
+ * number of stack slots used from the bottom of the stack.
+ */
+#include "lj_obj.h"
+
+static int cur_slots = -1;
+
+static int fill_stack(lua_State *L)
+{
+	cur_slots = L->base - tvref(L->stack);
+
+	while(lua_gettop(L) < LUAI_MAXSTACK) {
+		cur_slots += 1;
+		lua_pushinteger(L, 42);
+	}
+
+	return 0;
+}
+
+static int premature_stackoverflow(void *test_state)
+{
+	lua_State *L = test_state;
+	lua_cpcall(L, fill_stack, NULL);
+	assert_true(cur_slots == LUAI_MAXSTACK - 1);
+	return TEST_EXIT_SUCCESS;
+}
+
+/*
+ * XXX: This test should fail neither before the patch
+ * nor after it.
+ */
+static int stackoverflow_during_stackoverflow(void *test_state)
+{
+	lua_State *L = test_state;
+	/*
+	 * XXX: `fill_stack` acts here as its own error handler,
+	 * causing the second stack overflow.
+	 */
+	lua_pushcfunction(L, fill_stack);
+	lua_pushcfunction(L, fill_stack);
+	int status = lua_pcall(L, 0, 0, -2);
+	assert_true(status == LUA_ERRERR);
+	return TEST_EXIT_SUCCESS;
+}
+
+int main(void)
+{
+	lua_State *L = utils_lua_init();
+	const struct test_unit tgroup[] = {
+		test_unit_def(premature_stackoverflow),
+		test_unit_def(stackoverflow_during_stackoverflow),
+	};
+	const int test_result = test_run_group(tgroup, L);
+	utils_lua_close(L);
+	return test_result;
+}
-- 
2.43.0

[Tarantool-patches] [PATCH luajit v4 3/3] Follow-up fix for stack overflow handling cleanup.

[Maxim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

(cherry-picked from commit aa6b15c1a8922848bd6f596ba384824ca3fe0f5f)

The stack overflow error is thrown in `lj_state_growstack` only
if the coroutine status is `OK`, however, stack overflow can
happen on a yielded coroutine too. This patch fixes the condition
for status, so now the error thrown on yielded coroutines too.

Maxim Kokryashkin:
* added the description and the test for the patch

Part of tarantool/tarantool#9145
---
 src/lj_state.c                                |  2 +-
 .../lj-962-premature-stack-overflow.test.c    | 32 +++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/src/lj_state.c b/src/lj_state.c
index d8a5134c..01d4901a 100644
--- a/src/lj_state.c
+++ b/src/lj_state.c
@@ -126,7 +126,7 @@ void LJ_FASTCALL lj_state_growstack(lua_State *L, MSize need)
     if (L->stacksize > LJ_STACK_MAXEX)
       lj_err_throw(L, LUA_ERRERR);  /* Does not invoke an error handler. */
     /* 1. We are _at_ the limit after the last growth. */
-    if (!L->status) {  /* 2. Throw 'stack overflow'. */
+    if (L->status < LUA_ERRRUN) {  /* 2. Throw 'stack overflow'. */
       L->status = LUA_ERRRUN;  /* Prevent ending here again for pushed msg. */
       lj_err_msg(L, LJ_ERR_STKOV);  /* May invoke an error handler. */
     }
diff --git a/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c b/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
index 12cb9004..3b006805 100644
--- a/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
+++ b/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
@@ -7,7 +7,9 @@
 /*
  * XXX: The "lj_obj.h" header is included to calculate the
  * number of stack slots used from the bottom of the stack.
+ * XXX: The "lj_arch.h" header is included for the skipcond.
  */
+#include "lj_arch.h"
 #include "lj_obj.h"
 
 static int cur_slots = -1;
@@ -24,6 +26,22 @@ static int fill_stack(lua_State *L)
 	return 0;
 }
 
+#if !LJ_NO_UNWIND
+static int immediate_yield(lua_State *L)
+{
+	return lua_yield(L, 0);
+}
+
+static int overflow_suspended_coro(lua_State *L)
+{
+	lua_State *newL = lua_newthread(L);
+	lua_pushcfunction(newL, immediate_yield);
+	lua_resume(newL, 0);
+	fill_stack(newL);
+	return 0;
+}
+#endif
+
 static int premature_stackoverflow(void *test_state)
 {
 	lua_State *L = test_state;
@@ -50,12 +68,26 @@ static int stackoverflow_during_stackoverflow(void *test_state)
 	return TEST_EXIT_SUCCESS;
 }
 
+static int stackoverflow_on_suspended_coro(void *test_state)
+{
+#if LJ_NO_UNWIND
+	UNUSED(test_state);
+	return skip("Internal unwinding can't catch this exception");
+#else
+	lua_State *L = test_state;
+	int status = lua_cpcall(L, overflow_suspended_coro, NULL);
+	assert_true(status == LUA_ERRRUN);
+	return TEST_EXIT_SUCCESS;
+#endif
+}
+
 int main(void)
 {
 	lua_State *L = utils_lua_init();
 	const struct test_unit tgroup[] = {
 		test_unit_def(premature_stackoverflow),
 		test_unit_def(stackoverflow_during_stackoverflow),
+		test_unit_def(stackoverflow_on_suspended_coro),
 	};
 	const int test_result = test_run_group(tgroup, L);
 	utils_lua_close(L);
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit v4 0/3] Improve error reporting on stack overflow

[Sergey Bronnikov via Tarantool-patches]

Hi, Max


LGTM patch series, thanks!


On 2/1/24 14:24, Maxim Kokryashkin wrote:
> Changes in v4:
> - Added skipcond for nounwind builds.
>
> Issues: https://github.com/luajit/luajit/issues/962
> https://github.com/tarantool/tarantool/issues/9145
>
> PR: https://github.com/tarantool/tarantool/pull/9380
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-962-error-reporting-on-stack-overflow
>
> Mike Pall (3):
>    Improve error reporting on stack overflow.
>    Cleanup stack overflow handling.
>    Follow-up fix for stack overflow handling cleanup.
>
>   src/lj_state.c                                | 15 ++-
>   .../lj-962-premature-stack-overflow.test.c    | 95 +++++++++++++++++++
>   .../lj-603-err-snap-restore.test.lua          |  1 +
>   .../lj-962-stack-overflow-report.test.lua     | 10 ++
>   .../lj-962-stack-overflow-report/script.lua   |  3 +
>   test/tarantool-tests/utils/exec.lua           | 15 ++-
>   6 files changed, 132 insertions(+), 7 deletions(-)
>   create mode 100644 test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
>   create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report.test.lua
>   create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report/script.lua
>
> --
> 2.43.0
>

Re: [Tarantool-patches] [PATCH luajit v4 0/3] Improve error reporting on stack overflow

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM!


-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v4 0/3] Improve error reporting on stack overflow

[Igor Munkin via Tarantool-patches]

Max,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/3.0 and
release/2.11.

On 01.02.24, Maxim Kokryashkin via Tarantool-patches wrote:
> Changes in v4:
> - Added skipcond for nounwind builds.
> 
> Issues: https://github.com/luajit/luajit/issues/962
> https://github.com/tarantool/tarantool/issues/9145
> 
> PR: https://github.com/tarantool/tarantool/pull/9380
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-962-error-reporting-on-stack-overflow
> 
> Mike Pall (3):
>   Improve error reporting on stack overflow.
>   Cleanup stack overflow handling.
>   Follow-up fix for stack overflow handling cleanup.
> 
>  src/lj_state.c                                | 15 ++-
>  .../lj-962-premature-stack-overflow.test.c    | 95 +++++++++++++++++++
>  .../lj-603-err-snap-restore.test.lua          |  1 +
>  .../lj-962-stack-overflow-report.test.lua     | 10 ++
>  .../lj-962-stack-overflow-report/script.lua   |  3 +
>  test/tarantool-tests/utils/exec.lua           | 15 ++-
>  6 files changed, 132 insertions(+), 7 deletions(-)
>  create mode 100644 test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
>  create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report.test.lua
>  create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report/script.lua
> 
> --
> 2.43.0
> 

-- 
Best regards,
IM