Hi, Sergey!

Thanks for the fixes! LGTM

see my comments below

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.

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)