Tarantool development patches archive
 help / color / mirror / Atom feed
From: Maksim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: tarantool-patches@dev.tarantool.org, sergeyb@tarantool.org,
	skaplun@tarantool.org, m.kokryashkin@tarantool.org
Subject: [Tarantool-patches] [PATCH luajit v3 2/3] Cleanup stack overflow handling.
Date: Wed, 22 Nov 2023 17:35:33 +0300	[thread overview]
Message-ID: <20231122143534.11330-3-max.kokryashkin@gmail.com> (raw)
In-Reply-To: <20231122143534.11330-1-max.kokryashkin@gmail.com>

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.39.3 (Apple Git-145)


  parent reply	other threads:[~2023-11-22 14:36 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-22 14:35 [Tarantool-patches] [PATCH luajit v3 0/3] Improve error reporting on stack overflow Maksim Kokryashkin via Tarantool-patches
2023-11-22 14:35 ` [Tarantool-patches] [PATCH luajit v3 1/3] " Maksim Kokryashkin via Tarantool-patches
2023-11-24 12:22   ` Sergey Bronnikov via Tarantool-patches
2023-12-06 15:07     ` Maxim Kokryashkin via Tarantool-patches
2024-01-18 13:18       ` Sergey Bronnikov via Tarantool-patches
2023-11-22 14:35 ` Maksim Kokryashkin via Tarantool-patches [this message]
2023-11-24 12:30   ` [Tarantool-patches] [PATCH luajit v3 2/3] Cleanup stack overflow handling Sergey Bronnikov via Tarantool-patches
2023-12-06 15:02     ` Maxim Kokryashkin via Tarantool-patches
2024-01-18 13:02       ` Sergey Bronnikov via Tarantool-patches
2023-11-22 14:35 ` [Tarantool-patches] [PATCH luajit v3 3/3] Follow-up fix for stack overflow handling cleanup Maksim Kokryashkin via Tarantool-patches
2023-11-24 12:34   ` Sergey Bronnikov via Tarantool-patches
2023-12-06 14:47     ` Maxim Kokryashkin via Tarantool-patches
2024-01-18 13:02       ` Sergey Bronnikov via Tarantool-patches
2023-11-23  6:25 ` [Tarantool-patches] [PATCH luajit v3 0/3] Improve error reporting on stack overflow Sergey Kaplun via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231122143534.11330-3-max.kokryashkin@gmail.com \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit v3 2/3] Cleanup stack overflow handling.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox