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 523C616280A8; Tue, 9 Dec 2025 18:04:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 523C616280A8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1765292645; bh=ISoNuaFVfsL+t5vLuOQZx/aAdI0yZupHKujOoTUNo84=; 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=KXTzRdPZo/DKeG5tyGLrmZY6wFx1yJ97N6XmVTeuDSG9SPISLKIimqg/l4ZOyLum0 hRNzOHpxvwuVKVJSxsgzaV0v1SuEs0ZDPSQtmnHKuEp9SN/iDNXP7+nC4Px5QOEcd9 9ceAMhz6cfJlZcExJSM6lke5feq2zuq9kXXRIpoc= Received: from send149.i.mail.ru (send149.i.mail.ru [89.221.237.244]) (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 0DBC016280A6 for ; Tue, 9 Dec 2025 18:04:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0DBC016280A6 Received: by exim-smtp-9954f69f5-6blfq with esmtpa (envelope-from ) id 1vSzFv-000000003MI-3LJg; Tue, 09 Dec 2025 18:04:00 +0300 Content-Type: multipart/alternative; boundary="------------6Ly9nlpbz6IdfjfKPt0sxPpC" Message-ID: <9dc75e20-5ea5-47e4-8b1f-1d43e1aacae0@tarantool.org> Date: Tue, 9 Dec 2025 18:03:59 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org References: <68b23f93086762d93ae1089c466259477c6c1f81.1756287598.git.sergeyb@tarantool.org> In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92D4E0D2F4C74F725130936EA03A1FEC592EE2DB8BF78DF53182A05F538085040EFD3BCC17C6C4C3A3DE06ABAFEAF67050D0C85176F54B316509BBC3D1CC48F6D4D4D0ABEC86BDBF1 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE792E16514AF283DFAEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB55337566FF53983331E576C7E3C20FD23E293C5E6E1F812C2BC225387811BD7F7AB6539F389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0D9442B0B5983000E8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6D52CD31C43BF465FCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C22491638054B7D09EC083AA81AA40904B5D9CF19DD082D7633A0C84D3B47A649675F3AA81AA40904B5D98AA50765F790063765D77594429BFCE5EC76A7562686271ED91E3A1F190DE8FD2E808ACE2090B5E14AD6D5ED66289B5259CC434672EE63711DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3517C622C16A6DF10089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: 4965CFDFE05191347903E2BF2CCE50FB42605BC54A9F883B79311020FFC8D4ADBC5166BB1B1A8CB7DC182BE516452D02 X-C1DE0DAB: 0D63561A33F958A5F0377D467375F7535002B1117B3ED696E9A7C50E66CC1EBE466072E6821086B3823CB91A9FED034534781492E4B8EEADF89FF9AE94B7D007 X-C8649E89: 1C3962B70DF3F0AD73CAD6646DEDE191716CD42B3DD1D34C2AFAD3E4DDF5968D25B6776AC983F447FC0B9F89525902EE6F57B2FD27647F25E66C117BDB76D6591C2B55523F8FE5F6ED4035DAE7CC648A4F6D6069E00602125690EA156A85955898F0F8725C5EFF33B8341EE9D5BE9A0AAD3A823C6383F986C3B2E6DEDD07AAA5902937DECC87A3726536EB022892E5344C41F94D744909CE2512F26BEC029E55448553D2254B8D95CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVZ7tsNvok/TzKS7x3W7H1yg= X-Mailru-Sender: 811C44EDE0507D1FF7A5115BD94F839324B5DEE1E99892826750B362712FA5BD65820079B3C1E4F623A23C9E1D0981CD645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------6Ly9nlpbz6IdfjfKPt0sxPpC Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Sergey, thanks for review! Please consider my comments below. Sergey On 9/1/25 16:36, Sergey Kaplun via Tarantool-patches wrote: > 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 ("...") Fixed. > >> stack overflow in `pcall()`/`xpcall()` was fixed partially and > This handles stack overflow for vararg functions and metamethod > invocations not `xpcall()/pcall()` (directly). Updated: > In the previous commit ("LJ_FR2: Fix stack checks in vararg calls.") >  stack overflow for vararg functions and metamethod invocations >  was fixed partially and there are still cases where stack overflow >  happens, see comments in the test. The patch fixes the issue by >  adding the stack check to fast functions `pcall()` and `xpcall()`. >> 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. Added. >> 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)] >> | cmpNARGS8: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 >> | cmpNARGS8: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 >> | subsNARGS8: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) >> | beqzNARGS8: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) >> |. addiuNARGS8: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) >> | daddiuNARGS8:RC,NARGS8:RC, -8 >> - | lbu TMP3, DISPATCH_GL(hookmask)(DISPATCH) >> | bltzNARGS8: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.\ Done. > >> @@ -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) >> | daddiuNARGS8:RC,NARGS8:RC, -16 >> - | ld CARG1, 0(BASE) >> | ld CARG2, 8(BASE) >> | bltzNARGS8: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 >> | cmplwiNARGS8: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 >> | cmplwiNARGS8: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 >> + | movL:RB, SAVE_L >> + | lea RA, [BASE+NARGS:RD*8] >> + | cmp RA,L:RB->maxstack; ja ->fff_fallback >> | lea RA, [BASE+16] >> | subNARGS:RDd, 1 >> | mov PCd, 16+FRAME_PCALL >> @@ -1563,6 +1566,9 @@ static void build_subroutines(BuildCtx *ctx) >> | jmp ->vm_call_dispatch >> | >> |.ffunc_2 xpcall >> + | movL:RB, SAVE_L >> + | lea RA, [BASE+NARGS:RD*8] >> + | cmp RA,L:RB->maxstack; ja ->fff_fallback >> | movLFUNC:RA, [BASE+8] >> | checktp_ncLFUNC:RA, LJ_TFUNC, ->fff_fallback >> | movLFUNC: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 >> + | movL:RB, SAVE_L >> + | lea RA, [BASE+NARGS:RD*8] >> + | cmp RA,L:RB->maxstack; ja ->fff_fallback >> | lea RA, [BASE+8] >> | subNARGS:RD, 1 >> | mov PC, 8+FRAME_PCALL >> @@ -1925,6 +1928,9 @@ static void build_subroutines(BuildCtx *ctx) >> | jmp ->vm_call_dispatch >> | >> |.ffunc_2 xpcall >> + | movL: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. this testcase is failed too: TAP version 13 1..4 ok - no stack overflow with recursive pcall ok - no stack overflow with metamethod ================================================================= ==3374==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x007f77c03fc8 at pc 0x005574fc4fb0 bp 0x007ff2e0f3f0  sp 0x007ff2e0f410 WRITE of size 8 at 0x007f77c03fc8 thread T0     #0 0x5574fc4faf in copyTV /root/sergeyb/luajit/src/lj_obj.h:1000     #1 0x5574fc4faf in lj_meta_call /root/sergeyb/luajit/src/lj_meta.c:444     #2 0x557510c293 in lj_vmeta_call (/root/sergeyb/luajit/build/src/luajit+0x1d5293)     #3 0x5574f92a9b in lua_pcall /root/sergeyb/luajit/src/lj_api.c:1173     #4 0x5574f74217 in docall /root/sergeyb/luajit/src/luajit.c:134     #5 0x5574f75067 in handle_script /root/sergeyb/luajit/src/luajit.c:304     #6 0x5574f76d07 in pmain /root/sergeyb/luajit/src/luajit.c:602     #7 0x557510bb33 in lj_BC_FUNCC (/root/sergeyb/luajit/build/src/luajit+0x1d4b33)     #8 0x5574f937fb in lua_cpcall /root/sergeyb/luajit/src/lj_api.c:1208     #9 0x5574f77083 in main /root/sergeyb/luajit/src/luajit.c:633     #10 0x7f7dbfe79f in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x2079f)     #11 0x5574f73aa3 (/root/sergeyb/luajit/build/src/luajit+0x3caa3) > >> +-- 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? the test was removed > >> + 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. Added. > >> + 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 >> --------------6Ly9nlpbz6IdfjfKPt0sxPpC Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Sergey,

thanks for review! Please consider my comments below.

Sergey

On 9/1/25 16:36, Sergey Kaplun via Tarantool-patches wrote:
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 ("...")
Fixed. 


stack overflow in `pcall()`/`xpcall()` was fixed partially and

This handles stack overflow for vararg functions and metamethod
invocations not `xpcall()/pcall()` (directly).

Updated:

> In the previous commit ("LJ_FR2: Fix stack checks in vararg calls.")             
>  stack overflow for vararg functions and metamethod invocations                   
>  was fixed partially and there are still cases where stack overflow               
>  happens, see comments in the test. The patch fixes the issue by                  
>  adding the stack check to fast functions `pcall()` and `xpcall()`.               
 


      

        
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.


    

    Added.
    

      

      

        
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.\

    

    Done.
    

      



      

        
@@ -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.

    

    
this testcase is failed too:

    
TAP version 13                                                   
                     

      1..4                                                             
                             

      ok - no stack overflow with recursive pcall                       
               

      ok - no stack overflow with metamethod                           
          

      ================================================================= 
                          

      ==3374==ERROR: AddressSanitizer: heap-buffer-overflow on address
      0x007f77c03fc8 at pc 0x005574fc4fb0 bp 0x007ff2e0f3f0

       sp 0x007ff2e0f410                                               
            

      WRITE of size 8 at 0x007f77c03fc8 thread T0                       
       

          #0 0x5574fc4faf in copyTV
      /root/sergeyb/luajit/src/lj_obj.h:1000       

          #1 0x5574fc4faf in lj_meta_call
      /root/sergeyb/luajit/src/lj_meta.c:444

          #2 0x557510c293 in lj_vmeta_call
      (/root/sergeyb/luajit/build/src/luajit+0x1d5293)

          #3 0x5574f92a9b in lua_pcall
      /root/sergeyb/luajit/src/lj_api.c:1173  

          #4 0x5574f74217 in docall
      /root/sergeyb/luajit/src/luajit.c:134

          #5 0x5574f75067 in handle_script
      /root/sergeyb/luajit/src/luajit.c:304          

          #6 0x5574f76d07 in pmain
      /root/sergeyb/luajit/src/luajit.c:602   

          #7 0x557510bb33 in lj_BC_FUNCC
      (/root/sergeyb/luajit/build/src/luajit+0x1d4b33)

          #8 0x5574f937fb in lua_cpcall
      /root/sergeyb/luajit/src/lj_api.c:1208                        

          #9 0x5574f77083 in main /root/sergeyb/luajit/src/luajit.c:633

          #10 0x7f7dbfe79f in __libc_start_main
      (/lib/aarch64-linux-gnu/libc.so.6+0x2079f)

          #11 0x5574f73aa3 
      (/root/sergeyb/luajit/build/src/luajit+0x3caa3)

      

    

    

      



      

        
+-- 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?

    

    the test was removed
    

      



      

        
+  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.

    

    Added.
    

      



      

        
+  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



      

      

    

  
  


--------------6Ly9nlpbz6IdfjfKPt0sxPpC--