Hello, Max
thanks for the patch!
See a couple of minor comments below
Sergey
On 11/22/23 17:35, 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.
>
> 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;
> +}
> +
this testcase should fail with reverted patch, right? but it is not