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!
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(),
===================================================================

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

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

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

-- 
Best regards,
Sergey Kaplun