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

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

Hi, Maxim!
Just an additional nit below.

On 20.11.23, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Maxim!
> Thanks for the patch!
> Please consider my comments below.
> 
> On 18.11.23, 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.
> > 
> > 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..0873636a
> > --- /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);
> 
> Can we just use `LJ_GC64 + 1` with a comment here? It helps to avoid
> usage of internal objects.
> 
> > +
> > +	while(lua_gettop(L) < LUAI_MAXSTACK) {
> > +		cur_slots += 1;
> > +		lua_pushinteger(L, 42);
> > +	}
> > +
> > +	return 1;

What is the value returned here (on Lua stack), do we need it?

> > +}
> > +
> > +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),
> 
> This test doesn't check the case when we do not invoke an error handler
> (case 4).
> 
> Also, there is no check for the error handler itself to be called (it
> may be done in a separate test). OTOH, I'm not sure that this should be
> done as a part of this particular test. I suggest waiting for the
> Sergey's opinion here.
> 
> Also, I suggest adding a test when without set `L->status = LUA_ERRRUN`
> we are in trouble.
> 
> > +	};
> > +	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

-- 
Best regards,
Sergey Kaplun