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 7A3AED64442; Thu, 5 Sep 2024 19:24:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7A3AED64442 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1725553450; bh=7dp6jHOnsKkLUuYf/ddx/UyCE1zu9+K9JmM2eDurJyc=; 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=xi7w2coqYlNuEhUy/Cp1MFK7lIpxlsI0yRDcr5XuyulqSPuh8FBQ1h4Bxdh2y3Jao BY/0xS2zHhJji39Op2BJGFg0Cv0KROpGsaXh3bmtUk8Om+NpZvSQYry+draY4GFhEZ 55UHpoAs0RouvZTYDEM7I+YgDpfJeCPtXNfIzRC8= Received: from smtp16.i.mail.ru (smtp16.i.mail.ru [95.163.41.69]) (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 0A0A15BE742 for ; Thu, 5 Sep 2024 19:24:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0A0A15BE742 Received: by smtp16.i.mail.ru with esmtpa (envelope-from ) id 1smFHE-0000000ALIk-1AMb; Thu, 05 Sep 2024 19:24:08 +0300 Content-Type: multipart/alternative; boundary="------------9soVFI9KD2UtmJ7CXEDHihdl" Message-ID: <45594c65-9a3e-42ce-86c6-dd72c46b00fb@tarantool.org> Date: Thu, 5 Sep 2024 19:24:07 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun References: <20240826102520.29541-1-skaplun@tarantool.org> <5af61bfe-8827-40b4-b69c-17b5dd67a58f@tarantool.org> In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD978F4AA4899E1B8B58F22A8BB507A0B04F0A7E0C7045D83B2182A05F538085040C0F9A9B5F2025774C591814E25D11F9FE242B031C5A6E3652104AD8124079ACB29B867418450F35B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7589DF9800DB1E191EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063788D52F6B124E194E8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8BEC0E60424F9E0213576F18E3F5DD69D48D95DDE6E79DD7BCC7F00164DA146DAFE8445B8C89999728AA50765F790063783E00425F71A4181389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC821E93C0F2A571C7BF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CA6C7FFFE744CA7FBAD7EC71F1DB884274AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3705B0E1A13332C17BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7742996B5449390BC731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5833911D8CA72263E5002B1117B3ED696A027867A0F812E75361FAC1196A180DE823CB91A9FED034534781492E4B8EEAD2B25D9E4C92BC8ACBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFE30AA9002616880BF65D830D8B75170435FA6A5F62957B75AB715BBAB8F5BF97136DDA45E01CE4BED850D60E7C387E46A3C50D94665BB01D4DA41C5E35A3481E7782605CCDE475C85F4332CA8FE04980913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj2SEsQEvWBk4yJHvLwo9oJA== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F2588458FB0286196AC90F71741D06F8E7BD5C4996B7C924BDF214D84F243288C8BA1E4E645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------9soVFI9KD2UtmJ7CXEDHihdl Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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. But I'll not insist. > >>> +test:is(result, expected, 'correct result') >>> + >>> +test:done(true) --------------9soVFI9KD2UtmJ7CXEDHihdl Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

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 <mike>

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 is https://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.

    
But I'll not insist.

    

    

      



      

        

          
+test:is(result, expected, 'correct result')
+
+test:done(true)


        

      

      

    

  
  


--------------9soVFI9KD2UtmJ7CXEDHihdl--