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 D7A4CD6444D; Thu, 5 Sep 2024 20:14:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D7A4CD6444D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1725556497; bh=dIDO/blSizcggLY7NIDUC7BMgsEwIuugXzZJ12MsiyU=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=Dk4Ie3iMc10Ca2y1913h16d7HPh27q9BB0ZFvH9KUD19TLiXcUGC053y5WY0DZ2p8 e03qy7WS4yZZi8Pum98TVjke7pcq2GKYr/nnMT/h9UhQkE5HucDhZRWSGr2yOuSsRe kDYLHmOioB6RX1VRkEhdtTRsKBxjPcqyKUFnuef8= Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [95.163.41.93]) (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 6EC4BD6444D for ; Thu, 5 Sep 2024 20:14:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6EC4BD6444D Received: by smtp55.i.mail.ru with esmtpa (envelope-from ) id 1smG4N-0000000FGuG-24Sz; Thu, 05 Sep 2024 20:14:55 +0300 Date: Thu, 5 Sep 2024 20:14:48 +0300 To: Sergey Bronnikov Message-ID: References: <20240826102520.29541-1-skaplun@tarantool.org> <5af61bfe-8827-40b4-b69c-17b5dd67a58f@tarantool.org> <45594c65-9a3e-42ce-86c6-dd72c46b00fb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <45594c65-9a3e-42ce-86c6-dd72c46b00fb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD90CF8E60E6BB262B74AA35C611D93875AEA4A9CA610E28ABD182A05F5380850404C228DA9ACA6FE272CF1FC99460604E0411046492FDDF8065B0B30C8266BAB69E7A850AA24CD1C5C9CAD6AA93C8D19D4 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE796563325B6E011C8C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE79B25D085E9A31DCF8F08D7030A58E5AD1A62830130A00468AEEEE3FBA3A834EE7353EFBB553375660B018AD77D864BAF30C1A902DD4D2646E480C590B336A0D7EC6BD89EAAD4BD64389733CBF5DBD5E913377AFFFEAFD269A417C69337E82CC2CC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92176DF2183F8FC7C04CF195F1528592878941B15DA834481F9449624AB7ADAF372E808ACE2090B5E14AD6D5ED66289B5278DA827A17800CE76631511D42670FFE2EB15956EA79C166176DF2183F8FC7C04E672349037D5FA5725E5C173C3A84C3C9EEE74C166EF7BC35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5081178423D9B2F995002B1117B3ED696F4C7F84371A0364BF5FEB6EB1EB183FD823CB91A9FED034534781492E4B8EEAD8D8BB953E4894305BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF4745D21FADE697F99679FF2A29EEB61864946684CF791B6CFA94F94F9173DBE80F85117B495E935BD850D60E7C387E46C1DA1A57F96D3EE1D0516874DFAA6F2767DAAF900C1D28595F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj2SEsQEvWBk5MUEJlsDUmYQ== X-DA7885C5: E8FE606E43BF6C74F255D290C0D534F9A9129D71AC7E5993582CF07C681588DCE08862B96D1866B95B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393C6D0B12EA33CAA9B676A2314B70DF87317CDD26DDD9BDED251FFACD4B58A5A89E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Limit number of string format elements to compile. 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 Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Fixed your comments and force-pushed the branch. On 05.09.24, Sergey Bronnikov wrote: > Hi, Sergey! > > Thanks for the fixes! LGTM > > see my comments below > > On 04.09.2024 18:02, Sergey Kaplun wrote: > > Hi, Sergey! > > Thanks for the review! > > See my answers below. > > > > On 04.09.24, Sergey Bronnikov wrote: > >> Hi, Sergey, > >> > >> thanks for the patch! > >> > >> See my comments below. > >> > >> Sergey > >> > >> On 26.08.2024 13:25, Sergey Kaplun wrote: > >>> From: Mike Pall > >>> > >>> Reported by pwnhacker0x18. > >>> > >>> (cherry picked from commit 4fc48c50fe3f3f5a9680bada5c0c0d0d7eb345a3) > >>> > >>> When compiling `string.format()` with a huge sequence of elements, it is > >>> possible that too many KGC IRs underflow the IR buffer. This patch > >>> limits the maximum number of `string.format()` elements to be compiled > >>> to 100. > >>> > >>> Sergey Kaplun: > >>> * added the description and the test for the problem > >>> > >>> Part of tarantool/tarantool#10199 > >>> --- > >>> > >>> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1203-limit-format-elements > >>> Related issues: > >>> *https://github.com/tarantool/tarantool/issues/10199 > >>> *https://github.com/LuaJIT/LuaJIT/issues/1203 > >>> > >>> src/lj_ffrecord.c | 2 ++ > >>> .../lj-1203-limit-format-elements.test.lua | 28 +++++++++++++++++++ > >>> 2 files changed, 30 insertions(+) > >>> create mode 100644 test/tarantool-tests/lj-1203-limit-format-elements.test.lua > >>> > >>> diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c > >>> index d5fc081e..3b82d044 100644 > >>> --- a/src/lj_ffrecord.c > >>> +++ b/src/lj_ffrecord.c > >>> @@ -962,6 +962,7 @@ static void LJ_FASTCALL recff_string_format(jit_State *J, RecordFFData *rd) > >>> TRef hdr, tr; > >>> FormatState fs; > >>> SFormat sf; > >>> + int nfmt = 0; > >>> /* Specialize to the format string. */ > >>> emitir(IRTG(IR_EQ, IRT_STR), trfmt, lj_ir_kstr(J, fmt)); > >>> tr = hdr = recff_bufhdr(J); > >>> @@ -1031,6 +1032,7 @@ static void LJ_FASTCALL recff_string_format(jit_State *J, RecordFFData *rd) > >>> recff_nyiu(J, rd); > >>> return; > >>> } > >>> + if (++nfmt > 100) lj_trace_err(J, LJ_TRERR_TRACEOV); > >>> } > >>> J->base[0] = emitir(IRT(IR_BUFSTR, IRT_STR), tr, hdr); > >>> } > >>> diff --git a/test/tarantool-tests/lj-1203-limit-format-elements.test.lua b/test/tarantool-tests/lj-1203-limit-format-elements.test.lua > >>> new file mode 100644 > >>> index 00000000..f17d4e37 > >>> --- /dev/null > >>> +++ b/test/tarantool-tests/lj-1203-limit-format-elements.test.lua > >>> @@ -0,0 +1,28 @@ > >>> +local tap = require('tap') > >>> + > >>> +-- Test file to demonstrate LuaJIT incorrect recording of > >>> +-- `string.format()` function with huge amount of elements. > >>> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1173. > >> Seems a correct link ishttps://github.com/LuaJIT/LuaJIT/issues/1203 > > Fixed, thanks! > > Branch is force-pushed. > > > > =================================================================== > > diff --git a/test/tarantool-tests/lj-1203-limit-format-elements.test.lua b/test/tarantool-tests/lj-1203-limit-format-elements.test.lua > > index f17d4e37..bf250000 100644 > > --- a/test/tarantool-tests/lj-1203-limit-format-elements.test.lua > > +++ b/test/tarantool-tests/lj-1203-limit-format-elements.test.lua > > @@ -2,7 +2,7 @@ local tap = require('tap') > > > > -- Test file to demonstrate LuaJIT incorrect recording of > > -- `string.format()` function with huge amount of elements. > > --- See also:https://github.com/LuaJIT/LuaJIT/issues/1173. > > +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1203. > > > > local test = tap.test('lj-1203-limit-format-elements'):skipcond({ > > ['Test requires JIT enabled'] = not jit.status(), > > =================================================================== > Thanks! > >>> + > >>> +local test = tap.test('lj-1203-limit-format-elements'):skipcond({ > >>> + ['Test requires JIT enabled'] = not jit.status(), > >>> +}) > >>> + > >>> +test:plan(2) > >>> + > >>> +jit.opt.start('hotloop=1') > >>> + > >>> +-- XXX: Use a huge amount of format elements to process, which > >>> +-- creates a lot of string constants. > >>> +local NELEMENTS = 25000 > >> Why 25000? It is reproduced with 10000 as well. > > It is flaky-reproducible with less amount inside our test suite (at > > least on my laptop), so I prefer to keep this number of elements. > > > Okay > >> > >>> +local fmt = ('%'):rep(NELEMENTS * 2) > >>> +local expected = ('%'):rep(NELEMENTS) > >>> +local result > >>> +for _ = 1, 4 do > >>> + result = fmt:format() > >>> +end > >>> + > >>> +test:ok(true, 'no IR buffer underflow') > >> Why do you need this check? Why the following check it not enough? > > We usually check both (no assertion and correctness) where it is easily > > done. > > Looks like the first one is excessive and I would remove it. Fair enough. I've removed the first check. See the iterative patch below. Branch is force-pushed. =================================================================== diff --git a/test/tarantool-tests/lj-1203-limit-format-elements.test.lua b/test/tarantool-tests/lj-1203-limit-format-elements.test.lua index bf250000..d143b3fa 100644 --- a/test/tarantool-tests/lj-1203-limit-format-elements.test.lua +++ b/test/tarantool-tests/lj-1203-limit-format-elements.test.lua @@ -8,7 +8,7 @@ local test = tap.test('lj-1203-limit-format-elements'):skipcond({ ['Test requires JIT enabled'] = not jit.status(), }) -test:plan(2) +test:plan(1) jit.opt.start('hotloop=1') @@ -22,7 +22,6 @@ for _ = 1, 4 do result = fmt:format() end -test:ok(true, 'no IR buffer underflow') -test:is(result, expected, 'correct result') +test:is(result, expected, 'no IR buffer underflow and the correct result') test:done(true) =================================================================== > > But I'll not insist. > > > > >>> +test:is(result, expected, 'correct result') > >>> + > >>> +test:done(true) -- Best regards, Sergey Kaplun