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

[Tarantool-patches] [PATCH luajit v2 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

Changes in v2:
- Fixed comments as per review by Sergey.

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     |  9 +++
 .../lj-962-stack-overflow-report/script.lua   |  2 +
 test/tarantool-tests/utils/exec.lua           | 15 ++++-
 6 files changed, 99 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.39.3 (Apple Git-145)

[Tarantool-patches] [PATCH luajit v2 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.

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         |  9 +++++++++
 .../lj-962-stack-overflow-report/script.lua       |  2 ++
 test/tarantool-tests/utils/exec.lua               | 15 ++++++++++++---
 5 files changed, 25 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..26a90f8d
--- /dev/null
+++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
@@ -0,0 +1,9 @@
+local tap = require('tap')
+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..52734f2c
--- /dev/null
+++ b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
@@ -0,0 +1,2 @@
+-- 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..7ed0a9d8 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 cmd_start_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[cmd_start_idx(args)]
+end
+
+function M.luacmd(args)
+  -- Return the full command with flags.
+  return table.concat(args, ' ', cmd_start_idx(args), -1)
 end
 
 local function makeenv(tabenv)
-- 
2.39.3 (Apple Git-145)

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM, after fixing a few nits below.

On 22.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

Minor: s/xpcall/`xpcall`/
Feel free to ignore.

> 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         |  9 +++++++++
>  .../lj-962-stack-overflow-report/script.lua       |  2 ++
>  test/tarantool-tests/utils/exec.lua               | 15 ++++++++++++---
>  5 files changed, 25 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

<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..26a90f8d
> --- /dev/null
> +++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
> @@ -0,0 +1,9 @@
> +local tap = require('tap')

Minor: Please add a comment that the test reproduces the problem only
for GC64 mode (and 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..52734f2c
> --- /dev/null
> +++ b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
> @@ -0,0 +1,2 @@
> +-- luacheck: no global

Nit: It's better to add a comment about avoiding an additional stack
slot usage (local).

> +f = function() f() end; f()
> diff --git a/test/tarantool-tests/utils/exec.lua b/test/tarantool-tests/utils/exec.lua
> index a56ca2dc..7ed0a9d8 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 cmd_start_idx(args)

Nit: I suggest naming this function like `executable_idx()` or something
similar.

>    -- 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[cmd_start_idx(args)]
> +end
> +
> +function M.luacmd(args)
> +  -- Return the full command with flags.
> +  return table.concat(args, ' ', cmd_start_idx(args), -1)
>  end
>  
>  local function makeenv(tabenv)
> -- 
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
Sergey Kaplun

[Tarantool-patches] [PATCH luajit v2 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.

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    | 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..fd7557a7
--- /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 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;
+}
+
+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)

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

[Sergey Kaplun via Tarantool-patches]

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

On 22.11.23, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
> 
> Reported by Peter Cawley.
> 
> (cherry-picked from commit d2f6c55b05c716e5dbb479b7e684abaee7cf6e12)
> 

<snipped>

> ---
>  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. */

I suggest covering this special case in our test.

> +    /* 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)

<snipped>

>  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..fd7557a7
> --- /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"

Minor: Please provide rationalization for "lj_obj.h" used here (a
comment about calculation of stack usage is enough).

> +#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 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;
> +}
> +
> +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)
> 

-- 
Best regards,
Sergey Kaplun

[Tarantool-patches] [PATCH luajit v2 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 fd7557a7..39a95998 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 0;
 }
 
+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;
+}
+
 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 v2 3/3] Follow-up fix for stack overflow handling cleanup.

[Sergey Kaplun via Tarantool-patches]

Hi, Maksim!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun