Re: [Tarantool-patches] [PATCH luajit] Limit number of string format elements to compile.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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