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: 3093 bytes --]

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


> +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?
> +test:is(result, expected, 'correct result')
> +
> +test:done(true)

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