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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 7357 bytes --]

Thanks for changes! LGTM

On 12/6/23 18:02, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the review!
>
>     Пятница, 24 ноября 2023, 15:30 +03:00 от Sergey Bronnikov
>     <sergeyb@tarantool.org>:
>     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
>
> And it does fail. Tested on GC64/non-GC64 builds on Linux/MacOS.
>
>     > +/*
>     > + * XXX: This test should fail neither before the patch
>     > + * nor after it.
>
>     I propose to say about it in commit message.
>
> Fixed, the branch is force-pushed. New commit message:
> ====
> Cleanup stack overflow handling.
> 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.
> The `stackoverflow_during_stackoverflow` should fail neither
> before the patch nor after and is added for the test to be
> exhaustive.
> Maxim Kokryashkin:
> * added the description and the test for the problem
> Part of tarantool/tarantool#9145
> ====
>
>
>     We have a rule that test must fail without backported patch, so passed
>     test is unexpected here.
>
>     > + */
>     > +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;
>     > +}
>

[-- Attachment #2: Type: text/html, Size: 13131 bytes --]