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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.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
> +/*
> + * XXX: This test should fail neither before the patch
> + * nor after it.

I propose to say about it in commit message.

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