<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey!</p>
<p>Thanks for the fixes! LGTM</p>
<p>see my comments below<br>
</p>
<div class="moz-cite-prefix">On 04.09.2024 18:02, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:Zth2btaUMvW2n1IN@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the review!
See my answers below.
On 04.09.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hi, Sergey,
thanks for the patch!
See my comments below.
Sergey
On 26.08.2024 13:25, Sergey Kaplun wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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:<a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/tree/skaplun/lj-1203-limit-format-elements">https://github.com/tarantool/luajit/tree/skaplun/lj-1203-limit-format-elements</a>
Related issues:
*<a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/10199">https://github.com/tarantool/tarantool/issues/10199</a>
*<a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1203">https://github.com/LuaJIT/LuaJIT/issues/1203</a>
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:<a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1173">https://github.com/LuaJIT/LuaJIT/issues/1173</a>.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Seems a correct link is <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1203">https://github.com/LuaJIT/LuaJIT/issues/1203</a>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1173">https://github.com/LuaJIT/LuaJIT/issues/1173</a>.
+-- See also: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1203">https://github.com/LuaJIT/LuaJIT/issues/1203</a>.
local test = tap.test('lj-1203-limit-format-elements'):skipcond({
['Test requires JIT enabled'] = not jit.status(),
===================================================================
</pre>
</blockquote>
Thanks!<br>
<blockquote type="cite" cite="mid:Zth2btaUMvW2n1IN@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Why 25000? It is reproduced with 10000 as well.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
Okay<br>
<blockquote type="cite" cite="mid:Zth2btaUMvW2n1IN@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+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')
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Why do you need this check? Why the following check it not enough?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
We usually check both (no assertion and correctness) where it is easily
done.</pre>
</blockquote>
<p>Looks like the first one is excessive and I would remove it.</p>
<p>But I'll not insist.<br>
</p>
<blockquote type="cite" cite="mid:Zth2btaUMvW2n1IN@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+test:is(result, expected, 'correct result')
+
+test:done(true)
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>