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 63CB71BB78C8; Thu, 12 Mar 2026 20:18:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 63CB71BB78C8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1773335928; bh=shgJYQ6AuPP8Pue1dxZGbF58DdLQl2ae0myYvUj832U=; 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=uxJaF888qOq+wAyv0mlOk9vf4PBEssIs2bBXUyAA/HiHpgKCWjE4D+nWd3kHExwWY +ZLs8Fg+X7Pp9ao3/sZG+M5ABUGJufATDCTZC6OSb89YsPOTgiFMSLh9Cfc21wlWuO LVfaNZNhntKkIVdLjYDC8e8ducc0eULeAWsYQLmY= Received: from send104.i.mail.ru (send104.i.mail.ru [89.221.237.199]) (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 14D911BB78C6 for ; Thu, 12 Mar 2026 20:18:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 14D911BB78C6 Received: by exim-smtp-64cdfc6c8d-lxgsz with esmtpa (envelope-from ) id 1w0jgM-00000000SsQ-0HhB; Thu, 12 Mar 2026 20:18:46 +0300 Date: Thu, 12 Mar 2026 20:19:40 +0300 To: Sergey Bronnikov Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Message-ID: References: <1cc101f2-173e-47af-b373-cfb47868f313@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1cc101f2-173e-47af-b373-cfb47868f313@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD91ABAE9865AC7DC88E3B882EBCA423B4D6BA2A0DD5046E6A3182A05F53808504084177E214561919C3DE06ABAFEAF67057694B8F72FA7828F5697EB80B0DC2BBA16070DCDF568964E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE711269A7C2F827F16EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB5533756627194F95AE6E7C2DC87536CD07370A00919868BC734205261E9A22ED102FB8F0389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0A3E989B1926288338941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6D52CD31C43BF465FCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249E629FEBEEC38437D76E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8B211A43C7E0359BF03AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735D2D576BCF940C736C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5A4DB9601AEF22C055002B1117B3ED6964A352A12B8E87737406406D89DD9EB8A823CB91A9FED034534781492E4B8EEAD2B25D9E4C92BC8ACBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0AD73CAD6646DEDE191716CD42B3DD1D34CAB70F9BE574AE9C625B6776AC983F447FC0B9F89525902EE6F57B2FD27647F25E66C117BDB76D659F37DA3899513CE3DA0E226BC1A43CA36252DC257F067E6CAC6A1DABDD33F7E2DB431759721AA4642B8341EE9D5BE9A0A5A2681AC9D0DE90F3315B7BB5068B9EE4699CAADE11BF1F86536EB022892E5344C41F94D744909CECFA6C6B0C050A61A8CAF69B82BA93681CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVdbVVJCphTR/gRWNT2vaWPY= X-Mailru-Sender: 583F1D7ACE8F49BDD951BA70C165859EF8F492654EE0027508AF9BFB128101EEEB16FC390EF768E3D90F9C72B55308C5F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A84198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 3/3][v3] 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 fixes! LGTM, after fixing the last nit below. On 12.03.26, Sergey Bronnikov wrote: > Hi, Sergey, > > thanks for review! See my comments below. > > Sergey > > On 3/12/26 13:16, Sergey Kaplun via Tarantool-patches wrote: > > Hi, Sergey! > > Thanks for the patch! > > Please, fix my comments below. > > > > Don't forget to add the corresponding iterative changes. > > > > On 12.03.26, Sergey Bronnikov wrote: > >> From: Mike Pall > >> > >> Analyzed by Peter Cawley. > >> > >> (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36) > >> > >> The patch adds the stack check to fast functions `pcall()` and > >> `xpcall()`. > > Please add more verbose description: > > > > | (cherry picked from commit a4c1640432a9d8a60624cdc8065b15078c228e36) > > | > > | The `pcall()` and `xpcall()` calls in GC64 mode require 2 slots. This > > | means that all arguments should be moved up during emitting of the frame > > | link to the stack. Hence, this may cause stack overflow without the > > | corresponding check. > > | > > | This patch adds the corresponding checks to the VM. Non-GC64 VMs are > > | updated as well for the consistency. > Updated > >> Sergey Bronnikov: > >> * added the description and the test for the problem > >> > >> Part of tarantool/tarantool#12134 > >> --- > >> src/vm_arm.dasc | 7 ++++ > >> src/vm_arm64.dasc | 8 +++++ > >> src/vm_mips.dasc | 10 +++++- > >> src/vm_mips64.dasc | 14 ++++++-- > >> src/vm_ppc.dasc | 9 +++++ > >> src/vm_x64.dasc | 6 ++++ > >> src/vm_x86.dasc | 6 ++++ > >> ...048-fix-stack-checks-vararg-calls.test.lua | 35 ++++++++++++++++++- > >> 8 files changed, 90 insertions(+), 5 deletions(-) > >> > >> diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc > >> index 6c2975b4..4e60ee07 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 > >> @@ -1440,8 +1444,12 @@ static void build_subroutines(BuildCtx *ctx) > >> |. nop > >> | > >> |.ffunc xpcall > >> - | daddiuNARGS8:TMP0,NARGS8:RC, -16 > > This neglets the first patch in the series. See the comment below. > > > >> - | ld CARG1, 0(BASE) > >> + | 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 > > This line is incorrect. This neglets the 1st patch in the series. > > > > It should be > > | | daddiuNARGS8:TMP0,NARGS8:RC, -16 > > Right. However, probably we should leave this line near ".ffunc xpcall". > What do you think? Why? This break the `maxstack` check (since the RC is differs before the addition with TMP2). See the latest LuaJIT version: https://github.com/LuaJIT/LuaJIT/blob/659a61693aa3b87661864ad0f12eee14c865cd7f/src/vm_mips64.dasc#L1450 > > Now updated as the following: > > --- a/src/vm_mips64.dasc > +++ b/src/vm_mips64.dasc > @@ -1449,7 +1449,7 @@ static void build_subroutines(BuildCtx *ctx) >    |  sltu AT, TMP1, TMP2 >    |  bnez AT, ->fff_fallback >    |.  ld CARG1, 0(BASE) > -  |  daddiu NARGS8:RC, NARGS8:RC, -16 > +  |  daddiu NARGS8:TMP0, NARGS8:RC, -16 >    |   ld CARG2, 8(BASE) >    |    bltz NARGS8:TMP0, ->fff_fallback >    |.    lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH) > > > > >> | ld CARG2, 8(BASE) > >> | bltzNARGS8:TMP0, ->fff_fallback > >> |. lbu TMP1, DISPATCH_GL(hookmask)(DISPATCH) > >> 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 3a8ad63d..ad8b151b 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 > >> @@ -5,7 +5,7 @@ local tap = require('tap') > >> -- See alsohttps://github.com/LuaJIT/LuaJIT/issues/1048. > >> local test = tap.test('lj-1048-fix-stack-checks-vararg-calls') > >> > >> -test:plan(2) > >> +test:plan(5) > >> > >> -- The test case demonstrates a segmentation fault due to stack > >> -- overflow by recursive calling `pcall()`. The functions are > >> @@ -50,4 +50,37 @@ pcall(coroutine.wrap(looper), prober_2, 0) > >> > >> test:ok(true, 'no stack overflow with metamethod') > >> > >> +-- The testcases demonstrates a stack overflow in > >> +-- `pcall()`/xpcall()` triggered using metamethod `__call`. > >> + > >> +t = coroutine.wrap(setmetatable)({}, { __call = pcall }) > > I've meant the following: > > > > | t = setmetatable({}, { __call = pcall }) > > | coroutine.wrap(function() t() end)() > > > Updated > > @@ -53,7 +53,8 @@ test:ok(true, 'no stack overflow with metamethod') >  -- The testcases demonstrates a stack overflow in >  -- `pcall()`/xpcall()` triggered using metamethod `__call`. > > -t = coroutine.wrap(setmetatable)({}, { __call = pcall }) > +t = setmetatable({}, { __call = pcall }) > +coroutine.wrap(function() t() end)() > > test:ok(true, 'no stack overflow with metamethod __call with pcall()') > > > >> + > >> +test:ok(true, 'no stack overflow with metamethod __call with pcall()') > >> + > >> +t = coroutine.wrap(setmetatable)({}, { __call = xpcall }) > > I've meant the following: > > > > | t = setmetatable({}, { __call = xpcall }) > > | coroutine.wrap(function() t() end)() > > > > But this won't work since the second amount of xpcall must be the > > function. So, this test case is invalid. We must to duplicate the second > > approach with `xpcall()` > > > > This works fine. > > | LUA_PATH="src/?.lua;;" gdb --args src/luajit -e ' > > | local t = {} > > | local function xpcall_wrapper() > > | return xpcall(unpack(t)) > > | end > > | > > | local N_ITERATIONS = 200 > > | > > | for i = 1, N_ITERATIONS do > > | t[i], t[i + 1], t[i + 2] = xpcall, type, {} > > | coroutine.wrap(xpcall_wrapper)() > > | end > > | ' > > Updated: > > 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 6395dfaa..825568f9 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 > @@ -58,7 +58,17 @@ coroutine.wrap(function() t() end)() > > test:ok(true, 'no stack overflow with metamethod __call with pcall()') > > -t = coroutine.wrap(setmetatable)({}, { __call = xpcall }) > +t = {} > +local function xpcall_wrapper() > +  return xpcall(unpack(t)) > +end > + > +local N_ITERATIONS_1 = 200 Why do we need two variables with the same value of iterations? Let's use N_ITERATIONS with the comment for xpcall and pcall. > + > +for i = 1, N_ITERATIONS_1 do > +  t[i], t[i + 1], t[i + 2] = xpcall, type, {} > +  coroutine.wrap(xpcall_wrapper)() > +end > > test:ok(true, 'no stack overflow with metamethod __call with xpcall()') > > @@ -67,19 +77,19 @@ test:ok(true, 'no stack overflow with metamethod > __call with xpcall()') >  -- triggered using `unpack()`. > >  t = {} > -local function f() > +local function pcall_wrapper() >    return pcall(unpack(t)) >  end > > --- The problem is only reproduced on LuaJIT GC64 and is best > +-- The problem is only reproduced on LuaJIT GC64 and is better >  -- reproduced under Valgrind than AddressSanitizer. The chosen >  -- value was found experimentally and always results in an attempt >  -- to write beyond the allocated memory. > -local N_ITERATIONS = 200 > +local N_ITERATIONS_2 = 200 > > -for i = 1, N_ITERATIONS do > +for i = 1, N_ITERATIONS_2 do >    t[i], t[i + 1], t[i + 2] = pcall, type, {} > -  coroutine.wrap(f)() > +  coroutine.wrap(pcall_wrapper)() >  end > > test:ok(true, 'no stack overflow with unpacked pcalls') > > >> + > >> +test:ok(true, 'no stack overflow with metamethod __call with xpcall()') > >> + > >> +-- The testcase demonstrates a stack overflow in > >> +-- `pcall()`/`xpcall()` similar to the first testcase, but it is > >> +-- triggered using `unpack()`. > >> + > >> +t = {} > >> +local function f() > >> -- > >> 2.43.0 > >> -- Best regards, Sergey Kaplun