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 BBB8D6E742A; Mon, 20 Nov 2023 16:50:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BBB8D6E742A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1700488227; bh=/jyQscM4mh+c57kd9odRMU3p94PgXn0h9Ga5gAe/2kE=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=tTpmhcp5i2+UZeLvwwveqZwwMsydk92Zdeb+48u4ZlMlAXvF2cGwjJsPOVT1ezkMQ 3dxKErncYibGYnskAiFesY7n/AwK3pwm5Vi7sCEo9Jfx6oPY/5ILNGCu97SLT9HHbM VYcDfid2cx8TI29oepw4aH+7+U93l26LsqW2J1A8= Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [95.163.41.64]) (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 2668C6E7427 for ; Mon, 20 Nov 2023 16:50:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2668C6E7427 Received: by smtp41.i.mail.ru with esmtpa (envelope-from ) id 1r54fS-007qu4-14; Mon, 20 Nov 2023 16:50:26 +0300 Date: Mon, 20 Nov 2023 16:45:53 +0300 To: Maksim Kokryashkin , tarantool-patches@dev.tarantool.org Message-ID: References: <20231117234116.60037-1-max.kokryashkin@gmail.com> <20231117234116.60037-3-max.kokryashkin@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD93F1575C7510F5547A7B188C2A0C623B6189D854FF5608E9100894C459B0CD1B93F98CF983493180B4B0BDB3FEAEBC38A8B9F23DDA556435419F33B7625D3E01E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7C6068CE86C2B75F5EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637575C0790A70C4B158638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D81B27B88C3CC88403F4CBE113CFC0646F117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC60CDF180582EB8FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD1828451B159A507268D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE140C956E756FBB7A7F16001415B11694D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE3E753FA5741D1AD02040F9FF01DFDA4A8C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB861051D4BA689FC2E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F0A35B161A8BF67C1156CCFE7AF13BCA4B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5540A211264B4243F93EB5B0ABFCF3DFEC6EC20BF4CFA3F44F87CCE6106E1FC07E67D4AC08A07B9B0AD0E433DBF1FBFA3CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFD949FA49E7254EB210A9CDD57D11135E9CEAD602D308D446228FD4B2A3FE694FF09515A9A3E4BB8D88AD3775A6B0C834CD75180F2B533927EE37E8370605C47840B08DDBC5CE87D002C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojmRIjd71J0y22edj2pB7Usw== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A7699573E658A21AF7A57F39DA9683230067816609AA8FC7A761DEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim! Just an additional nit below. On 20.11.23, Sergey Kaplun via Tarantool-patches wrote: > Hi, Maxim! > Thanks for the patch! > Please consider my comments below. > > On 18.11.23, 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. > > > > 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 | 41 +++++++++++++++++++ > > 2 files changed, 52 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..0873636a > > --- /dev/null > > +++ b/test/tarantool-c-tests/lj-962-premature-stack-overflow.test.c > > @@ -0,0 +1,41 @@ > > +#include "lua.h" > > +#include "lauxlib.h" > > + > > +#include "test.h" > > +#include "utils.h" > > + > > +#include "lj_obj.h" > > +#include "luaconf.h" > > + > > +static int cur_slots = -1; > > + > > +static int fill_stack(lua_State *L) > > +{ > > + cur_slots = L->base - tvref(L->stack); > > Can we just use `LJ_GC64 + 1` with a comment here? It helps to avoid > usage of internal objects. > > > + > > + while(lua_gettop(L) < LUAI_MAXSTACK) { > > + cur_slots += 1; > > + lua_pushinteger(L, 42); > > + } > > + > > + return 1; What is the value returned here (on Lua stack), do we need it? > > +} > > + > > +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; > > +} > > + > > +int main(void) > > +{ > > + lua_State *L = utils_lua_init(); > > + const struct test_unit tgroup[] = { > > + test_unit_def(premature_stackoverflow), > > This test doesn't check the case when we do not invoke an error handler > (case 4). > > Also, there is no check for the error handler itself to be called (it > may be done in a separate test). OTOH, I'm not sure that this should be > done as a part of this particular test. I suggest waiting for the > Sergey's opinion here. > > Also, I suggest adding a test when without set `L->status = LUA_ERRRUN` > we are in trouble. > > > + }; > > + const int test_result = test_run_group(tgroup, L); > > + utils_lua_close(L); > > + return test_result; > > +} > > -- > > 2.39.3 (Apple Git-145) > > > > -- > Best regards, > Sergey Kaplun -- Best regards, Sergey Kaplun