<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Thanks for changes! LGTM<br>
</p>
<div class="moz-cite-prefix">On 12/6/23 18:02, Maxim Kokryashkin
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1701874979.797663853@f762.i.mail.ru">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div>Hi, Sergey!</div>
<div>Thanks for the review!</div>
<div> </div>
<blockquote
style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Пятница,
24 ноября 2023, 15:30 +03:00 от Sergey Bronnikov
<a class="moz-txt-link-rfc2396E" href="mailto:sergeyb@tarantool.org"><sergeyb@tarantool.org></a>:<br>
<div id="">
<div class="js-helper js-readmsg-msg">
<div>
<div id="style_17008290202031312327_BODY">Hello, Max<br>
<br>
thanks for the patch!<br>
<br>
See a couple of minor comments below<br>
<br>
Sergey<br>
<br>
On 11/22/23 17:35, Maksim Kokryashkin wrote:<br>
> From: Mike Pall <mike><br>
><br>
> Reported by Peter Cawley.<br>
><br>
> (cherry-picked from commit
d2f6c55b05c716e5dbb479b7e684abaee7cf6e12)<br>
><br>
> After the previous patch, it is possible to trigger
the<br>
> `stack overflow` error prematurely. Consider the
following<br>
> situation: there are already 33000 slots allocated
on a Lua<br>
> stack, and then there are 30 additional slots
needed. In this<br>
> case, the actual allocated amount would be twice
the already<br>
> allocated size, shrunk to the `LJ_STACK_MAXEX`
size, which<br>
> would lead to the stack overflow error, despite the
fact there<br>
> is plenty of unused space. This patch completely
reworks the<br>
> logic of error handling during stack growth to
address the issue.<br>
><br>
> Another important thing to notice is that the
`LJ_ERR_STKOV` is<br>
> thrown only if the `L->status` is `LUA_OK` and
that status is set<br>
> to `LUA_ERRRUN` just before throwing the error. The
status is set<br>
> to `LUA_ERRRUN` to avoid the second stack overflow
during the<br>
> `err_msgv` execution.<br>
><br>
> Maxim Kokryashkin:<br>
> * added the description and the test for the
problem<br>
><br>
> Part of tarantool/tarantool#9145<br>
> ---<br>
> src/lj_state.c | 15 +++--<br>
> .../lj-962-premature-stack-overflow.test.c | 63
+++++++++++++++++++<br>
> 2 files changed, 74 insertions(+), 4 deletions(-)<br>
> create mode 100644
test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c<br>
><br>
> diff --git a/src/lj_state.c b/src/lj_state.c<br>
> index 76153bad..d8a5134c 100644<br>
> --- a/src/lj_state.c<br>
> +++ b/src/lj_state.c<br>
> @@ -121,8 +121,17 @@ void
lj_state_shrinkstack(lua_State *L, MSize used)<br>
> void LJ_FASTCALL lj_state_growstack(lua_State *L,
MSize need)<br>
> {<br>
> MSize n;<br>
> - if (L->stacksize > LJ_STACK_MAXEX) /*
Overflow while handling overflow? */<br>
> - lj_err_throw(L, LUA_ERRERR);<br>
> + if (L->stacksize >= LJ_STACK_MAXEX) {<br>
> + /* 4. Throw 'error in error handling' when we are
_over_ the limit. */<br>
> + if (L->stacksize > LJ_STACK_MAXEX)<br>
> + lj_err_throw(L, LUA_ERRERR); /* Does not invoke
an error handler. */<br>
> + /* 1. We are _at_ the limit after the last
growth. */<br>
> + if (!L->status) { /* 2. Throw 'stack
overflow'. */<br>
> + L->status = LUA_ERRRUN; /* Prevent ending here
again for pushed msg. */<br>
> + lj_err_msg(L, LJ_ERR_STKOV); /* May invoke an
error handler. */<br>
> + }<br>
> + /* 3. Add space (over the limit) for pushed
message and error handler. */<br>
> + }<br>
> n = L->stacksize + need;<br>
> if (n > LJ_STACK_MAX) {<br>
> n += 2*LUA_MINSTACK;<br>
> @@ -132,8 +141,6 @@ void LJ_FASTCALL
lj_state_growstack(lua_State *L, MSize need)<br>
> n = LJ_STACK_MAX;<br>
> }<br>
> resizestack(L, n);<br>
> - if (L->stacksize >= LJ_STACK_MAXEX)<br>
> - lj_err_msg(L, LJ_ERR_STKOV);<br>
> }<br>
><br>
> void LJ_FASTCALL lj_state_growstack1(lua_State *L)<br>
> 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<br>
> new file mode 100644<br>
> index 00000000..12cb9004<br>
> --- /dev/null<br>
> +++
b/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c<br>
> @@ -0,0 +1,63 @@<br>
> +#include "lua.h"<br>
> +#include "lauxlib.h"<br>
> +<br>
> +#include "test.h"<br>
> +#include "utils.h"<br>
> +<br>
> +/*<br>
> + * XXX: The "lj_obj.h" header is included to
calculate the<br>
> + * number of stack slots used from the bottom of
the stack.<br>
> + */<br>
> +#include "lj_obj.h"<br>
> +<br>
> +static int cur_slots = -1;<br>
> +<br>
> +static int fill_stack(lua_State *L)<br>
> +{<br>
> + cur_slots = L->base - tvref(L->stack);<br>
> +<br>
> + while(lua_gettop(L) < LUAI_MAXSTACK) {<br>
> + cur_slots += 1;<br>
> + lua_pushinteger(L, 42);<br>
> + }<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int premature_stackoverflow(void
*test_state)<br>
> +{<br>
> + lua_State *L = test_state;<br>
> + lua_cpcall(L, fill_stack, NULL);<br>
> + assert_true(cur_slots == LUAI_MAXSTACK - 1);<br>
> + return TEST_EXIT_SUCCESS;<br>
> +}<br>
> +<br>
this testcase should fail with reverted patch, right?
but it is not</div>
</div>
</div>
</div>
</blockquote>
<div>And it does fail. Tested on GC64/non-GC64 builds on
Linux/MacOS.</div>
<blockquote
style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div>> +/*<br>
> + * XXX: This test should fail neither before the
patch<br>
> + * nor after it.<br>
<br>
I propose to say about it in commit message.</div>
</div>
</div>
</div>
</blockquote>
<div>Fixed, the branch is force-pushed. New commit message:</div>
<div>====</div>
<div><span
style="font-family: var(--vkui--octavius_font_family_global, var(--vkui--font_family_base, Helvetica, Arial, sans-serif)); letter-spacing: var(--vkui--font_text--letter_spacing--regular, normal);">Cleanup
stack overflow handling.</span>
<div> </div>
<div>
<div>Reported by Peter Cawley.</div>
</div>
<div> </div>
<div>
<div>(cherry-picked from commit
d2f6c55b05c716e5dbb479b7e684abaee7cf6e12)</div>
</div>
<div> </div>
<div>
<div>After the previous patch, it is possible to trigger the</div>
<div>`stack overflow` error prematurely. Consider the
following</div>
<div>situation: there are already 33000 slots allocated on a
Lua</div>
<div>stack, and then there are 30 additional slots needed. In
this</div>
<div>case, the actual allocated amount would be twice the
already</div>
<div>allocated size, shrunk to the `LJ_STACK_MAXEX` size,
which</div>
<div>would lead to the stack overflow error, despite the fact
there</div>
<div>is plenty of unused space. This patch completely reworks
the</div>
<div>logic of error handling during stack growth to address
the issue.</div>
</div>
<div> </div>
<div>
<div>Another important thing to notice is that the
`LJ_ERR_STKOV` is</div>
<div>thrown only if the `L->status` is `LUA_OK` and that
status is set</div>
<div>to `LUA_ERRRUN` just before throwing the error. The
status is set</div>
<div>to `LUA_ERRRUN` to avoid the second stack overflow during
the</div>
<div>`err_msgv` execution.</div>
</div>
<div> </div>
<div>
<div>The `stackoverflow_during_stackoverflow` should fail
neither</div>
<div>before the patch nor after and is added for the test to
be</div>
<div>exhaustive.</div>
</div>
<div> </div>
<div>
<div>Maxim Kokryashkin:</div>
<div>* added the description and the test for the problem</div>
</div>
<div> </div>
<div>
<div>Part of tarantool/tarantool#9145</div>
<div>====</div>
</div>
</div>
<blockquote
style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div><br>
We have a rule that test must fail without backported
patch, so passed<br>
test is unexpected here.<br>
<br>
> + */<br>
> +static int stackoverflow_during_stackoverflow(void
*test_state)<br>
> +{<br>
> + lua_State *L = test_state;<br>
> + /*<br>
> + * XXX: `fill_stack` acts here as its own error
handler,<br>
> + * causing the second stack overflow.<br>
> + */<br>
> + lua_pushcfunction(L, fill_stack);<br>
> + lua_pushcfunction(L, fill_stack);<br>
> + int status = lua_pcall(L, 0, 0, -2);<br>
> + assert_true(status == LUA_ERRERR);<br>
> + return TEST_EXIT_SUCCESS;<br>
> +}<br>
> +<br>
> +int main(void)<br>
> +{<br>
> + lua_State *L = utils_lua_init();<br>
> + const struct test_unit tgroup[] = {<br>
> + test_unit_def(premature_stackoverflow),<br>
> +
test_unit_def(stackoverflow_during_stackoverflow),<br>
> + };<br>
> + const int test_result = test_run_group(tgroup,
L);<br>
> + utils_lua_close(L);<br>
> + return test_result;<br>
> +}</div>
</div>
</div>
</div>
</blockquote>
<div> </div>
</blockquote>
</body>
</html>