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

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

[Maksim Kokryashkin via Tarantool-patches]

This patchset fixes error reporting on stack overflow for situation
when an xpcall error handler can use a bit more space.

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    | 64 +++++++++++++++++++
 .../lj-603-err-snap-restore.test.lua          |  1 +
 .../lj-962-stack-overflow-report.test.lua     | 24 +++++++
 4 files changed, 100 insertions(+), 4 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

--
2.39.3 (Apple Git-145)

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

[Maksim 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.

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     | 24 +++++++++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report.test.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..bcddff01
--- /dev/null
+++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
@@ -0,0 +1,24 @@
+local tap = require('tap')
+local test = tap.test('lj-962-stack-overflow-report')
+
+test:plan(2)
+
+-- luacheck: no unused
+local _, _, _, _, _, _, _
+local error_message
+local handler_is_called = false
+local function errfunc(err)
+  -- luacheck: no unused
+  local _
+  error_message = err
+  handler_is_called = true
+end
+
+local function recursive_f()
+  recursive_f()
+end
+xpcall(recursive_f, errfunc)
+
+test:ok(handler_is_called, 'error handler was able to use a stack slot')
+test:like(error_message, '.+stack overflow$', 'stack overflow happened')
+test:done(true)
-- 
2.39.3 (Apple Git-145)

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

[Maksim 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.

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    | 41 +++++++++++++++++++
 2 files changed, 52 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..0873636a
--- /dev/null
+++ b/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
@@ -0,0 +1,41 @@
+#include "lua.h"
+#include "lauxlib.h"
+
+#include "test.h"
+#include "utils.h"
+
+#include "lj_obj.h"
+#include "luaconf.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 1;
+}
+
+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;
+}
+
+int main(void)
+{
+	lua_State *L = utils_lua_init();
+	const struct test_unit tgroup[] = {
+		test_unit_def(premature_stackoverflow),
+	};
+	const int test_result = test_run_group(tgroup, L);
+	utils_lua_close(L);
+	return test_result;
+}
-- 
2.39.3 (Apple Git-145)

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

[Maksim 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    | 23 +++++++++++++++++++
 2 files changed, 24 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 0873636a..7a03038b 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
@@ -21,6 +21,20 @@ static int fill_stack(lua_State *L)
 	return 1;
 }
 
+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 1;
+}
+
 static int premature_stackoverflow(void *test_state)
 {
 	lua_State *L = test_state;
@@ -29,11 +43,20 @@ static int premature_stackoverflow(void *test_state)
 	return TEST_EXIT_SUCCESS;
 }
 
+static int stackoverflow_on_suspended_coro(void *test_state)
+{
+	lua_State *L = test_state;
+	int status = lua_cpcall(L, overflow_suspended_coro, NULL);
+	assert_true(status == LUA_ERRRUN);
+	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_on_suspended_coro),
 	};
 	const int test_result = test_run_group(tgroup, L);
 	utils_lua_close(L);
-- 
2.39.3 (Apple Git-145)

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

[Sergey Kaplun via Tarantool-patches]

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

On 18.11.23, Maksim Kokryashkin wrote:
> 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.
> 
> 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     | 24 +++++++++++++++++++
>  3 files changed, 26 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report.test.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

<snipped>

> 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

<snipped>

> 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..bcddff01
> --- /dev/null
> +++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
> @@ -0,0 +1,24 @@
> +local tap = require('tap')
> +local test = tap.test('lj-962-stack-overflow-report')
> +
> +test:plan(2)
> +
> +-- luacheck: no unused
> +local _, _, _, _, _, _, _

I am very nervous, when I see these `_, _, ...` guys again.
I suggest introducing helper to fill up the necessary slots with
correponding comments.

Also, this test passes on the GC64/non-GC64 build on the current master
branch.

> +local error_message
> +local handler_is_called = false
> +local function errfunc(err)
> +  -- luacheck: no unused
> +  local _
> +  error_message = err
> +  handler_is_called = true
> +end
> +
> +local function recursive_f()
> +  recursive_f()
> +end
> +xpcall(recursive_f, errfunc)
> +
> +test:ok(handler_is_called, 'error handler was able to use a stack slot')

Typo: s/was/is/

Minor: Also maybe it's worth being more descriptive "an additional stack
slot".

> +test:like(error_message, '.+stack overflow$', 'stack overflow happened')

Minor: I suggest dropping `.+` at the start of the pattern since it
doesn't matter. NB: the `$` is important to distinguish `STKOV` and
`STKOVM`.

> +test:done(true)
> -- 
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

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

On 18.11.23, Maksim Kokryashkin wrote:
> 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.
> 
> 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    | 41 +++++++++++++++++++
>  2 files changed, 52 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..0873636a
> --- /dev/null
> +++ b/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
> @@ -0,0 +1,41 @@
> +#include "lua.h"
> +#include "lauxlib.h"
> +
> +#include "test.h"
> +#include "utils.h"
> +
> +#include "lj_obj.h"
> +#include "luaconf.h"
> +
> +static int cur_slots = -1;
> +
> +static int fill_stack(lua_State *L)
> +{
> +	cur_slots = L->base - tvref(L->stack);

Can we just use `LJ_GC64 + 1` with a comment here? It helps to avoid
usage of internal objects.

> +
> +	while(lua_gettop(L) < LUAI_MAXSTACK) {
> +		cur_slots += 1;
> +		lua_pushinteger(L, 42);
> +	}
> +
> +	return 1;
> +}
> +
> +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;
> +}
> +
> +int main(void)
> +{
> +	lua_State *L = utils_lua_init();
> +	const struct test_unit tgroup[] = {
> +		test_unit_def(premature_stackoverflow),

This test doesn't check the case when we do not invoke an error handler
(case 4).

Also, there is no check for the error handler itself to be called (it
may be done in a separate test). OTOH, I'm not sure that this should be
done as a part of this particular test. I suggest waiting for the
Sergey's opinion here.

Also, I suggest adding a test when without set `L->status = LUA_ERRRUN`
we are in trouble.

> +	};
> +	const int test_result = test_run_group(tgroup, L);
> +	utils_lua_close(L);
> +	return test_result;
> +}
> -- 
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maksim!
Thanks for the patch!
LGTM, except an insignificant nit below.

On 18.11.23, Maksim Kokryashkin wrote:
> 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    | 23 +++++++++++++++++++
>  2 files changed, 24 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 0873636a..7a03038b 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
> @@ -21,6 +21,20 @@ static int fill_stack(lua_State *L)
>  	return 1;
>  }
>  
> +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 1;

Why do we return something from this Lua function? What is this value on
Lua stack?

> +}
> +
>  static int premature_stackoverflow(void *test_state)
>  {
>  	lua_State *L = test_state;
> @@ -29,11 +43,20 @@ static int premature_stackoverflow(void *test_state)
>  	return TEST_EXIT_SUCCESS;
>  }
>  
> +static int stackoverflow_on_suspended_coro(void *test_state)
> +{
> +	lua_State *L = test_state;
> +	int status = lua_cpcall(L, overflow_suspended_coro, NULL);
> +	assert_true(status == LUA_ERRRUN);
> +	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_on_suspended_coro),
>  	};
>  	const int test_result = test_run_group(tgroup, L);
>  	utils_lua_close(L);
> -- 
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Just an additional nit below.

On 20.11.23, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Maxim!
> Thanks for the patch!
> Please consider my comments below.
> 
> On 18.11.23, Maksim Kokryashkin wrote:
> > 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.
> > 
> > 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    | 41 +++++++++++++++++++
> >  2 files changed, 52 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..0873636a
> > --- /dev/null
> > +++ b/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c
> > @@ -0,0 +1,41 @@
> > +#include "lua.h"
> > +#include "lauxlib.h"
> > +
> > +#include "test.h"
> > +#include "utils.h"
> > +
> > +#include "lj_obj.h"
> > +#include "luaconf.h"
> > +
> > +static int cur_slots = -1;
> > +
> > +static int fill_stack(lua_State *L)
> > +{
> > +	cur_slots = L->base - tvref(L->stack);
> 
> Can we just use `LJ_GC64 + 1` with a comment here? It helps to avoid
> usage of internal objects.
> 
> > +
> > +	while(lua_gettop(L) < LUAI_MAXSTACK) {
> > +		cur_slots += 1;
> > +		lua_pushinteger(L, 42);
> > +	}
> > +
> > +	return 1;

What is the value returned here (on Lua stack), do we need it?

> > +}
> > +
> > +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;
> > +}
> > +
> > +int main(void)
> > +{
> > +	lua_State *L = utils_lua_init();
> > +	const struct test_unit tgroup[] = {
> > +		test_unit_def(premature_stackoverflow),
> 
> This test doesn't check the case when we do not invoke an error handler
> (case 4).
> 
> Also, there is no check for the error handler itself to be called (it
> may be done in a separate test). OTOH, I'm not sure that this should be
> done as a part of this particular test. I suggest waiting for the
> Sergey's opinion here.
> 
> Also, I suggest adding a test when without set `L->status = LUA_ERRRUN`
> we are in trouble.
> 
> > +	};
> > +	const int test_result = test_run_group(tgroup, L);
> > +	utils_lua_close(L);
> > +	return test_result;
> > +}
> > -- 
> > 2.39.3 (Apple Git-145)
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
Sergey Kaplun