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

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

[-- Attachment #1: Type: text/plain, Size: 4755 bytes --]

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.

But I'll not insist.

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

[-- Attachment #2: Type: text/html, Size: 6918 bytes --]