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 0ABDA14D620F; Mon, 1 Sep 2025 16:35:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0ABDA14D620F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1756733724; bh=HLb+E8pXzGxHeogoW+L2TyebAyfQMkwbNygQW1zh8tM=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=u2Moupyl0TkNEiit04Eayk4ZiSdT9xHXqkXJLJzwsYWEH0G+gnXxugZEIl6uuoSbX 3CU71MnYVP3V9oRiZ5ZxDxQEo2BJbAA1uUMPNggsqLRhUdukzQFn2gEg77naJ6TUHF A/GCBqY36k0vT/uvdQ22IfaS8RqF1ketpPtq78ic= Received: from send36.i.mail.ru (send36.i.mail.ru [89.221.237.131]) (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 D298E14D620F for ; Mon, 1 Sep 2025 16:35:22 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D298E14D620F Received: by exim-smtp-6944cbb85b-847qp with esmtpa (envelope-from ) id 1ut4gr-00000000Pk5-37jD; Mon, 01 Sep 2025 16:35:22 +0300 Date: Mon, 1 Sep 2025 16:36:06 +0300 To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Message-ID: References: <68b23f93086762d93ae1089c466259477c6c1f81.1756287598.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <68b23f93086762d93ae1089c466259477c6c1f81.1756287598.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9F7647FA575010556B5A1E406A4D1B053FF28C69A833E29AE182A05F53808504096FB61D71D82E10C3DE06ABAFEAF67053DE295F6E1EA82BB82D6363B54654B5CCEE9AC3790D7AB9F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7387B291F9AC4D188EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB55337566D328D16D1FE2D8A77340EFFF1595E832DF53553745F064BF7B2FC923279FFFD0389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0A3E989B1926288338941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B65FF0BFC5AEE34BE6CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB86D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE73349F3DFB73D9B11731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5DCD8131BFB7B533F5002B1117B3ED6962DE93C44263AD2A23E67C18142C611B7823CB91A9FED034534781492E4B8EEAD69BF13FED57427F1BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF6888826DBF4140C3044CD4069D797B925CC88F94F9636B63636C3D2CBBBE076145AE5861559675EDF492A9F576A244DC78961FCD2B4FEFD02765C8B8A2C8FBA40B1F819AB28CAD64111DC66A97D0BFE2913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVXNcwk+fZooKYnwIcoVi2lk= X-DA7885C5: 704DA69C271DA713F255D290C0D534F9497F20DA675BF0C6502245CAF40F9083F5B766DC6D297ED05B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393FE9E42A757851DB67BD6E22924FD832F5FEF122875D21C4859AD013CD8FAEAD4E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/2] Add stack check to pcall/xpcall. 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, Sergey! Thanks for the patch! Please consider my comments below. On 27.08.25, Sergey Bronnikov wrote: > Analyzed by Peter Cawley. > > (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36) > > In the commit "LJ_FR2: Fix stack checks in vararg calls." Minor: In the previous commit ("...") > stack overflow in `pcall()`/`xpcall()` was fixed partially and This handles stack overflow for vararg functions and metamethod invocations not `xpcall()/pcall()` (directly). > there are still cases where stack overflow happens, see comments > in the test. The patch add stack check to `pcall()` and `xpcall()`. Please, mention in the commit message that the issue was fixed by adding the stack check to these fast functions. > > Sergey Bronnikov: > * added the description and the test for the problem > > Part of tarantool/tarantool#11691 > --- > src/vm_arm.dasc | 7 +++++ > src/vm_arm64.dasc | 8 +++++ > src/vm_mips.dasc | 10 +++++- > src/vm_mips64.dasc | 12 +++++-- > src/vm_ppc.dasc | 9 ++++++ > src/vm_x64.dasc | 6 ++++ > src/vm_x86.dasc | 6 ++++ > ...048-fix-stack-checks-vararg-calls.test.lua | 31 ++++++++++++++++++- > 8 files changed, 85 insertions(+), 4 deletions(-) > > diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc > index 7095e660..efe9dcb2 100644 > --- a/src/vm_arm.dasc > +++ b/src/vm_arm.dasc > @@ -1201,8 +1201,11 @@ static void build_subroutines(BuildCtx *ctx) > |//-- Base library: catch errors ---------------------------------------- > | > |.ffunc pcall > + | ldr RB, L->maxstack > + | add INS, BASE, NARGS8:RC > | ldrb RA, [DISPATCH, #DISPATCH_GL(hookmask)] > | cmp NARGS8:RC, #8 > + | cmphs RB, INS > | blo ->fff_fallback > | tst RA, #HOOK_ACTIVE // Remember active hook before pcall. > | mov RB, BASE > @@ -1213,7 +1216,11 @@ static void build_subroutines(BuildCtx *ctx) > | b ->vm_call_dispatch > | > |.ffunc_2 xpcall > + | ldr RB, L->maxstack > + | add INS, BASE, NARGS8:RC > | ldrb RA, [DISPATCH, #DISPATCH_GL(hookmask)] > + | cmp RB, INS > + | blo ->fff_fallback > | checkfunc CARG4, ->fff_fallback // Traceback must be a function. > | mov RB, BASE > | strd CARG12, [BASE, #8] // Swap function and traceback. > diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc > index cf8e575a..53ff7162 100644 > --- a/src/vm_arm64.dasc > +++ b/src/vm_arm64.dasc > @@ -1166,6 +1166,10 @@ static void build_subroutines(BuildCtx *ctx) > |//-- Base library: catch errors ---------------------------------------- > | > |.ffunc pcall > + | ldr TMP1, L->maxstack > + | add TMP2, BASE, NARGS8:RC > + | cmp TMP1, TMP2 > + | blo ->fff_fallback > | cmp NARGS8:RC, #8 > | ldrb TMP0w, GL->hookmask > | blo ->fff_fallback > @@ -1185,6 +1189,10 @@ static void build_subroutines(BuildCtx *ctx) > | b ->vm_call_dispatch > | > |.ffunc xpcall > + | ldr TMP1, L->maxstack > + | add TMP2, BASE, NARGS8:RC > + | cmp TMP1, TMP2 > + | blo ->fff_fallback > | ldp CARG1, CARG2, [BASE] > | ldrb TMP0w, GL->hookmask > | subs NARGS8:TMP1, NARGS8:RC, #16 > diff --git a/src/vm_mips.dasc b/src/vm_mips.dasc > index 32caabf7..69d09d52 100644 > --- a/src/vm_mips.dasc > +++ b/src/vm_mips.dasc > @@ -1382,9 +1382,13 @@ static void build_subroutines(BuildCtx *ctx) > |//-- Base library: catch errors ---------------------------------------- > | > |.ffunc pcall > + | lw TMP1, L->maxstack > + | addu TMP2, BASE, NARGS8:RC > | lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH) > | beqz NARGS8:RC, ->fff_fallback > - | move TMP2, BASE > + |. sltu AT, TMP1, TMP2 > + | bnez AT, ->fff_fallback > + |. move TMP2, BASE > | addiu BASE, BASE, 8 > | // Remember active hook before pcall. > | srl TMP3, TMP3, HOOK_ACTIVE_SHIFT > @@ -1394,8 +1398,12 @@ static void build_subroutines(BuildCtx *ctx) > |. addiu NARGS8:RC, NARGS8:RC, -8 > | > |.ffunc xpcall > + | lw TMP1, L->maxstack > + | addu TMP2, BASE, NARGS8:RC > | sltiu AT, NARGS8:RC, 16 > | lw CARG4, 8+HI(BASE) > + | sltu TMP1, TMP1, TMP2 > + | or AT, AT, TMP1 > | bnez AT, ->fff_fallback > |. lw CARG3, 8+LO(BASE) > | lw CARG1, LO(BASE) > diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc > index 7f49df5b..06b143a2 100644 > --- a/src/vm_mips64.dasc > +++ b/src/vm_mips64.dasc > @@ -1418,8 +1418,12 @@ static void build_subroutines(BuildCtx *ctx) > |//-- Base library: catch errors ---------------------------------------- > | > |.ffunc pcall > + | ld TMP1, L->maxstack > + | daddu TMP2, BASE, NARGS8:RC > + | sltu AT, TMP1, TMP2 > + | bnez AT, ->fff_fallback > + |. lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH) > | daddiu NARGS8:RC, NARGS8:RC, -8 > - | lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH) > | bltz NARGS8:RC, ->fff_fallback > |. move TMP2, BASE > | daddiu BASE, BASE, 16 I see that the original patch in the upstream has another diff. Please backport the commit ea7071d3 ("MIPS64: Fix xpcall() error case") first (as the first commit in the patch series) to avoid conflicts in the future. > @@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx) > |. nop > | > |.ffunc xpcall > + | ld TMP1, L->maxstack > + | daddu TMP2, BASE, NARGS8:RC > + | sltu AT, TMP1, TMP2 > + | bnez AT, ->fff_fallback > + |. ld CARG1, 0(BASE) > | daddiu NARGS8:RC, NARGS8:RC, -16 > - | ld CARG1, 0(BASE) > | ld CARG2, 8(BASE) > | bltz NARGS8:RC, ->fff_fallback > |. lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH) > diff --git a/src/vm_ppc.dasc b/src/vm_ppc.dasc > index 980ad897..f2ea933b 100644 > --- a/src/vm_ppc.dasc > +++ b/src/vm_ppc.dasc > @@ -1755,8 +1755,12 @@ static void build_subroutines(BuildCtx *ctx) > |//-- Base library: catch errors ---------------------------------------- > | > |.ffunc pcall > + | lwz TMP1, L->maxstack > + | add TMP2, BASE, NARGS8:RC > | cmplwi NARGS8:RC, 8 > | lbz TMP3, DISPATCH_GL(hookmask)(DISPATCH) > + | cmplw cr1, TMP1, TMP2 > + | cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt > | blt ->fff_fallback > | mr TMP2, BASE > | la BASE, 8(BASE) > @@ -1767,14 +1771,19 @@ static void build_subroutines(BuildCtx *ctx) > | b ->vm_call_dispatch > | > |.ffunc xpcall > + | lwz TMP1, L->maxstack > + | add TMP2, BASE, NARGS8:RC > | cmplwi NARGS8:RC, 16 > | lwz CARG3, 8(BASE) > + | cmplw cr1, TMP1, TMP2 > |.if FPU > | lfd FARG2, 8(BASE) > + | cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt > | lfd FARG1, 0(BASE) > |.else > | lwz CARG1, 0(BASE) > | lwz CARG2, 4(BASE) > + | cror 4*cr0+lt, 4*cr0+lt, 4*cr1+lt > | lwz CARG4, 12(BASE) > |.endif > | blt ->fff_fallback > diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc > index d5296759..141f5f82 100644 > --- a/src/vm_x64.dasc > +++ b/src/vm_x64.dasc > @@ -1545,6 +1545,9 @@ static void build_subroutines(BuildCtx *ctx) > |//-- Base library: catch errors ---------------------------------------- > | > |.ffunc_1 pcall > + | mov L:RB, SAVE_L > + | lea RA, [BASE+NARGS:RD*8] > + | cmp RA, L:RB->maxstack; ja ->fff_fallback > | lea RA, [BASE+16] > | sub NARGS:RDd, 1 > | mov PCd, 16+FRAME_PCALL > @@ -1563,6 +1566,9 @@ static void build_subroutines(BuildCtx *ctx) > | jmp ->vm_call_dispatch > | > |.ffunc_2 xpcall > + | mov L:RB, SAVE_L > + | lea RA, [BASE+NARGS:RD*8] > + | cmp RA, L:RB->maxstack; ja ->fff_fallback > | mov LFUNC:RA, [BASE+8] > | checktp_nc LFUNC:RA, LJ_TFUNC, ->fff_fallback > | mov LFUNC:RB, [BASE] // Swap function and traceback. > diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc > index b043b830..1ba5abce 100644 > --- a/src/vm_x86.dasc > +++ b/src/vm_x86.dasc > @@ -1914,6 +1914,9 @@ static void build_subroutines(BuildCtx *ctx) > |//-- Base library: catch errors ---------------------------------------- > | > |.ffunc_1 pcall > + | mov L:RB, SAVE_L > + | lea RA, [BASE+NARGS:RD*8] > + | cmp RA, L:RB->maxstack; ja ->fff_fallback > | lea RA, [BASE+8] > | sub NARGS:RD, 1 > | mov PC, 8+FRAME_PCALL > @@ -1925,6 +1928,9 @@ static void build_subroutines(BuildCtx *ctx) > | jmp ->vm_call_dispatch > | > |.ffunc_2 xpcall > + | mov L:RB, SAVE_L > + | lea RA, [BASE+NARGS:RD*8] > + | cmp RA, L:RB->maxstack; ja ->fff_fallback > | cmp dword [BASE+12], LJ_TFUNC; jne ->fff_fallback > | mov RB, [BASE+4] // Swap function and traceback. > | mov [BASE+12], RB > diff --git a/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua > index e300d5c1..367aecb6 100644 > --- a/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua > +++ b/test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua > @@ -7,7 +7,7 @@ local test = tap.test('lj-1048-fix-stack-checks-vararg-calls'):skipcond({ > ['Test requires JIT enabled'] = not jit.status(), > }) > > -test:plan(2) > +test:plan(4) > > -- The first testcase demonstrate a stack overflow in `pcall()` > -- by recursive calling `pcall()`. The functions are vararg > @@ -53,4 +53,33 @@ pcall(coroutine.wrap(looper_2), 0) > > test:ok(true, 'no stack overflow with using metamethod') > Why do you drop the original test case? Expected behaviour: | src/luajit -e 'local t = {setmetatable({},{__call=pcall})()} print(t[#t])' | stack overflow Actual behaviour -- dirty read detected by ASAN. > +-- The third testcase demonstrate a stack overflow in > +-- `pcall()`/xpcall()` similar to the first testcase, but it is > +-- triggered using hand-crafted Lua chunk with a lot `pcall()` > +-- builtins. > + > +for i = 1, 100 do > + local code = 'return pcall(' .. string.rep('pcall, ', i) .. 'pairs, {})' Why do we need this test if it has the same semantics as the second one? > + local f = load(code) > + coroutine.wrap(f)() > +end > + > +test:ok(true, 'no stack overflow with pcalls in load()') > + > +-- The fourth testcase demonstrate a stack overflow in > +-- `pcall()`/`xpcall()` similar to the first testcase, but it is > +-- triggered using `unpack()`. > + > +local t = {} > +local function f() > + return pcall(unpack(t)) > +end > + > +for i = 1, 100 do This limit isn't enough for GC64 or non-GC64 mode. | src/luajit -e ' | local t = {} | local function f() return pcall(unpack(t)) end | for i = 1, 100 do | t[i], t[i+1], t[i+2] = pcall, pairs, {} | coroutine.wrap(f)() | end | ' For the GC64 build it is necessary to set the limit as 180, (179 -- not SegFault). Please provide two different limits depending on the GC64 mode configuration. Please, describe why the __exact__ limit is chosen for the particular configuration. > + t[i], t[i + 1], t[i + 2] = pcall, pairs, {} > + coroutine.wrap(f)() > +end > + > +test:ok(true, 'no stack overflow with unpacked pcalls') > + > test:done(true) > -- > 2.43.0 > -- Best regards, Sergey Kaplun