From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: Maksim Kokryashkin <max.kokryashkin@gmail.com>,
tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit v3 2/3] Cleanup stack overflow handling.
Date: Thu, 18 Jan 2024 16:02:43 +0300 [thread overview]
Message-ID: <8088e21f-b374-4449-bfe8-744531ad5964@tarantool.org> (raw)
In-Reply-To: <1701874979.797663853@f762.i.mail.ru>
[-- 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 --]
next prev parent reply other threads:[~2024-01-18 13:02 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-22 14:35 [Tarantool-patches] [PATCH luajit v3 0/3] Improve error reporting on stack overflow Maksim Kokryashkin via Tarantool-patches
2023-11-22 14:35 ` [Tarantool-patches] [PATCH luajit v3 1/3] " Maksim Kokryashkin via Tarantool-patches
2023-11-24 12:22 ` Sergey Bronnikov via Tarantool-patches
2023-12-06 15:07 ` Maxim Kokryashkin via Tarantool-patches
2024-01-18 13:18 ` Sergey Bronnikov via Tarantool-patches
2023-11-22 14:35 ` [Tarantool-patches] [PATCH luajit v3 2/3] Cleanup stack overflow handling Maksim Kokryashkin via Tarantool-patches
2023-11-24 12:30 ` Sergey Bronnikov via Tarantool-patches
2023-12-06 15:02 ` Maxim Kokryashkin via Tarantool-patches
2024-01-18 13:02 ` Sergey Bronnikov via Tarantool-patches [this message]
2023-11-22 14:35 ` [Tarantool-patches] [PATCH luajit v3 3/3] Follow-up fix for stack overflow handling cleanup Maksim Kokryashkin via Tarantool-patches
2023-11-24 12:34 ` Sergey Bronnikov via Tarantool-patches
2023-12-06 14:47 ` Maxim Kokryashkin via Tarantool-patches
2024-01-18 13:02 ` Sergey Bronnikov via Tarantool-patches
2023-11-23 6:25 ` [Tarantool-patches] [PATCH luajit v3 0/3] Improve error reporting on stack overflow Sergey Kaplun via Tarantool-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8088e21f-b374-4449-bfe8-744531ad5964@tarantool.org \
--to=tarantool-patches@dev.tarantool.org \
--cc=m.kokryashkin@tarantool.org \
--cc=max.kokryashkin@gmail.com \
--cc=sergeyb@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH luajit v3 2/3] Cleanup stack overflow handling.' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox