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 > : > 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 > > > > 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; > > +} >