

<HTML><BODY><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 <sergeyb@tarantool.org>:<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></BODY></HTML>