<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi, Sergey</p>
<p>thanks for review! See my comments.</p>
<p>New changes were force-pushed.<br>
</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 8/29/23 16:38, Sergey Kaplun wrote:<br>
</div>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On 29.08.23, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">From: <a class="moz-txt-link-abbreviated" href="mailto:sergeyb@tarantool.org">sergeyb@tarantool.org</a>
Reported by Sergey Bronnikov. #1054
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I suggest to remove the ticket number, to avoid trouble trouble until
trouble troubles you. :)</pre>
</blockquote>
<p>Agree, removed to avoid troubles.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
(cherry picked from commit 309fb42b871b6414f53e0e0e708bce0b0d62daff)
The following Lua snippet triggers an out of boundary access to a stack:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/an out of boundary/out-of-boundary/</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
```lua
a, b, c = 1, 2, 3
local d
for _ in nil do end
```
With execution snippet by LuaJIT instrumented by ASAN it leads to
a heap-buffer-overflow.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I suppose that it leads ever without ASAN, but the issue is
observable only with ASAN, isn't it?</pre>
</blockquote>
<p>Right. Rephrased it:</p>
<p><br>
</p>
<p>+-- The test demonstrates a problem with out-of-boundary access<br>
+-- to a stack. The problem can be easily observed on execution<br>
+-- the sample by LuaJIT by ASAN, sanitizer reports a
heap-buffer-overflow.<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
In a function `predict_next` variable `exprpc` looks forward and expects
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: I suggest using of `()` for distinguishing function and variable
names.
Feel free to ignore.</pre>
</blockquote>
<p>Fixed. However "function" was before "predict_next".<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">extra bytecodes on the stack. However, `KPRI` is merged to the `KNIL`
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/the `KNIL`/`KNIL`</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">and there is no new bytecode to add, so `exprpc == fs->bclim` and it
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: /fs->bclim`/fs->bclim`,/</pre>
</blockquote>
<p><br>
</p>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">leads to out of boundary access.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/out of boundary/out-of-boundary/</pre>
</blockquote>
<p><br>
</p>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: I suppose that we can mention that the patch fixes the issue via
early return.
</pre>
</blockquote>
<p>Added.<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Sergey Bronnikov:
* added the description and the test for the problem
Part of tarantool/tarantool#8825
---
PR: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/pull/9054">https://github.com/tarantool/tarantool/pull/9054</a>
Branch: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/tree/ligurio/lj-1054-incorrect-pc-value-predict_next">https://github.com/tarantool/luajit/tree/ligurio/lj-1054-incorrect-pc-value-predict_next</a>
Related issue:
* <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1054">https://github.com/LuaJIT/LuaJIT/issues/1054</a>
src/lj_parse.c | 4 +++-
...incorrect-pc-value-in-predict_next.test.lua | 18 ++++++++++++++++++
2 files changed, 21 insertions(+), 1 deletion(-)
create mode 100644 test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua
diff --git a/src/lj_parse.c b/src/lj_parse.c
index 343fa797..f1015960 100644
--- a/src/lj_parse.c
+++ b/src/lj_parse.c
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">diff --git a/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua b/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua
new file mode 100644
index 00000000..17f1b994
--- /dev/null
+++ b/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua
@@ -0,0 +1,18 @@
+local tap = require('tap')
+local test = tap.test('lj-1054-incorrect-pc-value-in-predict_next')
+test:plan(1)
+
+
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Excess empty line.</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+-- The test demonstrates a problem with out of boundary access to a stack.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/out of boundary/out-of-boundary/
Comment line width is more than 66 symbols.</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+-- Sample executed in LuaJIT instrumented by ASAN leads to
+-- a heap-buffer-overflow.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: IDK why, but suggested varian here is "heap buffer overflow".</pre>
</blockquote>
<p><br>
</p>
<p>ASAN reports error with hyphens, like this:</p>
<pre class="lang-c s-code-block" style="margin: 0px 0px calc(1.5em); padding: var(--su12); border: 0px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-variant-numeric: inherit; font-variant-east-asian: inherit; font-variant-alternates: inherit; font-weight: 400; font-stretch: inherit; line-height: var(--lh-md); font-family: var(--ff-mono); font-optical-sizing: inherit; font-kerning: inherit; font-feature-settings: inherit; font-variation-settings: inherit; font-size: var(--fs-body1); vertical-align: baseline; box-sizing: inherit; width: auto; max-height: 600px; overflow: auto; background-color: var(--highlight-bg); border-radius: var(--br-md); --_cb-line-numbers-bg: var(--black-050); color: var(--highlight-color); overflow-wrap: normal; letter-spacing: normal; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;"><code class="hljs language-c" style="margin: 0px; padding: 0px; border: 0px; font-style: inherit; font-variant: inherit; font-weight: inherit; font-stretch: inherit; line-height: inherit; font-family: inherit; font-optical-sizing: inherit; font-kerning: inherit; font-feature-settings: inherit; font-variation-settings: inherit; font-size: var(--_pr-code-fs); vertical-align: baseline; box-sizing: inherit; background-color: transparent; white-space: inherit;">==<span class="hljs-number" style="margin: 0px; padding: 0px; border: 0px; font-style: inherit; font-variant: inherit; font-weight: inherit; font-stretch: inherit; line-height: inherit; font-family: inherit; font-optical-sizing: inherit; font-kerning: inherit; font-feature-settings: inherit; font-variation-settings: inherit; font-size: 13px; vertical-align: baseline; box-sizing: inherit; color: var(--highlight-namespace);">90673</span>==ERROR: AddressSanitizer: heap-buffer-overflow on address <span class="hljs-number" style="margin: 0px; padding: 0px; border: 0px; font-style: inherit; font-variant: inherit; font-weight: inherit; font-stretch: inherit; line-height: inherit; font-family: inherit; font-optical-sizing: inherit; font-kerning: inherit; font-feature-settings: inherit; font-variation-settings: inherit; font-size: 13px; vertical-align: baseline; box-sizing: inherit; color: var(--highlight-namespace);">0x6020000000fb</span> at pc <span class="hljs-number" style="margin: 0px; padding: 0px; border: 0px; font-style: inherit; font-variant: inherit; font-weight: inherit; font-stretch: inherit; line-height: inherit; font-family: inherit; font-optical-sizing: inherit; font-kerning: inherit; font-feature-settings: inherit; font-variation-settings: inherit; font-size: 13px; vertical-align: baseline; box-sizing: inherit; color: var(--highlight-namespace);">0x000108868a95</span> bp <span class="hljs-number" style="margin: 0px; padding: 0px; border: 0px; font-style: inherit; font-variant: inherit; font-weight: inherit; font-stretch: inherit; line-height: inherit; font-family: inherit; font-optical-sizing: inherit; font-kerning: inherit; font-feature-settings: inherit; font-variation-settings: inherit; font-size: 13px; vertical-align: baseline; box-sizing: inherit; color: var(--highlight-namespace);">0x7fff573979a0</span> sp <span class="hljs-number" style="margin: 0px; padding: 0px; border: 0px; font-style: inherit; font-variant: inherit; font-weight: inherit; font-stretch: inherit; line-height: inherit; font-family: inherit; font-optical-sizing: inherit; font-kerning: inherit; font-feature-settings: inherit; font-variation-settings: inherit; font-size: 13px; vertical-align: baseline; box-sizing: inherit; color: var(--highlight-namespace);">0x7fff57397998</span>
READ of size <span class="hljs-number" style="margin: 0px; padding: 0px; border: 0px; font-style: inherit; font-variant: inherit; font-weight: inherit; font-stretch: inherit; line-height: inherit; font-family: inherit; font-optical-sizing: inherit; font-kerning: inherit; font-feature-settings: inherit; font-variation-settings: inherit; font-size: 13px; vertical-align: baseline; box-sizing: inherit; color: var(--highlight-namespace);">1</span> at <span class="hljs-number" style="margin: 0px; padding: 0px; border: 0px; font-style: inherit; font-variant: inherit; font-weight: inherit; font-stretch: inherit; line-height: inherit; font-family: inherit; font-optical-sizing: inherit; font-kerning: inherit; font-feature-settings: inherit; font-variation-settings: inherit; font-size: 13px; vertical-align: baseline; box-sizing: inherit; color: var(--highlight-namespace);">0x6020000000fb</span> thread T0</code></pre>
<p></p>
<p>If you don't like variant "heap-buffer-overflow" then we can use
variant used in CWE list: "heap-based buffer overflow", see [1].<br>
</p>
<p>What variant should <br>
</p>
<p>1. <a class="moz-txt-link-freetext" href="https://cwe.mitre.org/data/definitions/122.html">https://cwe.mitre.org/data/definitions/122.html</a><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+-- See also <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/528">https://github.com/LuaJIT/LuaJIT/issues/528</a>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I suggest to add an empty line here.</pre>
</blockquote>
Added.<br>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+local lua_code = [[
+a, b, c = 1, 2, 3
+local d
+for _ in nil do end
+]]
+
+test:ok(loadstring(lua_code), 'parsing is correct')
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I suggest also to test that the behaviour of the executed chunk is the
same as in the PUC RIO Lua 5.1 (like it is done for the lj-1033).</pre>
</blockquote>
<p>Updated:</p>
<p><br>
</p>
<p>TAP version 13<br>
1..3<br>
ok - chunk loaded successfully<br>
ok - loaded function is failed (expected)<br>
ok - correct error message<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZO308um2V51Y6b6S@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+test:done(true)
--
2.34.1
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
</html>