From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 65E90978F8F; Thu, 18 Jan 2024 16:02:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 65E90978F8F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1705582965; bh=RF5eyoqDM4SE8aM3P5VZFmSbAOtN0jMcoZ5jVWf02QM=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=siKtUh9hPbIag0jtY6XJxm5sc+68TaKY3wVBwHcsiVrOuh67+w/GcxN3v2ud0xTXJ yKVlvAmtcs8qYpUQE6EH8qExMD3+WSWFeFs6AyTdcqs0kJTLvjAr15i5YlCJx29amX 7lpROWqcOf+16vD/nBHDgx9Nl9OfZfZ6Ucy20qJ8= Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [95.163.41.72]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 62AA6978F8F for ; Thu, 18 Jan 2024 16:02:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 62AA6978F8F Received: by smtp31.i.mail.ru with esmtpa (envelope-from ) id 1rQS2d-00000003Whe-0VHb; Thu, 18 Jan 2024 16:02:43 +0300 Content-Type: multipart/alternative; boundary="------------EkL802rDqN0EBQ4KorulNkGY" Message-ID: <8088e21f-b374-4449-bfe8-744531ad5964@tarantool.org> Date: Thu, 18 Jan 2024 16:02:43 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Maxim Kokryashkin References: <20231122143534.11330-1-max.kokryashkin@gmail.com> <20231122143534.11330-3-max.kokryashkin@gmail.com> <1701874979.797663853@f762.i.mail.ru> In-Reply-To: <1701874979.797663853@f762.i.mail.ru> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9AE5B4AFB3AE2A590AC960D445231AB294ACF14C8196FF36C182A05F5380850405F9FE0DB55D1CA7CA6D5EE0DB6E1EC8D85257A1D5725E0324CBE91D84A52AAB643C662A976723F50 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CE4525FFB91B9BBCEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006372521E7C1CE72986C8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88CA980046D51315DC84EEBE303674DF09063DE8842FB26D2CC7F00164DA146DAFE8445B8C89999728AA50765F790063793270F7220657A0A389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8C2B5EEE3591E0D35F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C86A7C529F68B8E5CC0837EA9F3D197644AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3E9BD6149E9909983BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CFED8438A78DFE0A9E1DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C30B8B320EE581A76035872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5131DFE4621F51B6A5002B1117B3ED6965A0C0C1017EE8632361FAC1196A180DE823CB91A9FED034534781492E4B8EEAD0D89974173551D4FBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF91BAE61A69EF953FE7487A934EC945270834575A44CF584F8B4E2AFE67D6683AD1BAA24A4D7F85D86197189FEE4B1C7AA59994B53A53B2EACEE5400F2C249FC8278E3D9F4539BFF1C226CC413062362A913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojjkiC0IEfsqIYADRqG8wX4g== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F25884584D42BEB0194EFF6ECE7DA15FCD53A221E6275501D1A738547978D7D5DFF14C86645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v3 2/3] Cleanup stack overflow handling. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: Maksim Kokryashkin , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------EkL802rDqN0EBQ4KorulNkGY Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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; > > +} > --------------EkL802rDqN0EBQ4KorulNkGY Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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