<!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>Hello, Sergey,</p>
<p>thanks for review! See my answers below.<br>
</p>
<div class="moz-cite-prefix">On 12.08.2024 16:32, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:ZroO7BXtZTer6kka@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the fixes!
On 23.07.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hi,
please see comments below. Fixes applied and force-pushed.
Sergey
On 10.07.2024 16:13, Sergey Kaplun wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the fixes!
Please consider my minor nits about comments below.
On 09.07.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hi, Sergey,
thanks for review. Fixes applied and force-pushed.
Sergey
On 09.07.2024 14:52, Sergey Kaplun via Tarantool-patches wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On 09.07.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">From: Mike Pall <mike>
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
It is more correct to say, that "`lua_State` is marked after the
function is removed from it" (since we stop the GC before chunk
loading and starts after).
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
My bad:
Typo: s/starts/start/</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZroO7BXtZTer6kka@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Also, it is worth mentioning that the problem was partially solved, the
complete fix will be applied in the next patch.
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">Added.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Please, add its description to the commit message too.
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I would rephrase this part as the following:
| The patch fixes the problem partially by marking the finalizer table
| on the start of the GC cycle.
| The complete fix will be applied in the next patch by turning the
| finalizer table into the proper GC root.
</pre>
</blockquote>
Updated, thanks!<br>
<blockquote type="cite" cite="mid:ZroO7BXtZTer6kka@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Sergey Bronnikov:
* added the description and the tests for the problem
Part of tarantool/tarantool#10199
---
src/lj_gc.c | 3 +
...free-on-access-to-CTState-finalizer.test.c | 66 +++++++++++++++++++
...ee-on-access-to-CTState-finalizer.test.lua | 18 +++++
3 files changed, 87 insertions(+)
create mode 100644 test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
create mode 100644 test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ * has the finalizer table as its environment. But, there is no
+ * FFI module table anywhere to anchor the `ffi.gc` itself, and
+ * the `lua_State` object was marked before the function is
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">It is more correct to say, that "`lua_State` is marked after the
function is removed from it" (since we stop the GC before chunk
loading and starts after).
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
My bad:
Typo: s/starts/start/
</pre>
</blockquote>
<p>Updated:</p>
<p>---
a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c<br>
+++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c<br>
@@ -16,8 +16,8 @@<br>
* on-demand during the parsing of this number. After the FFI<br>
* library is open, `ffi.gc` has the finalizer table as its<br>
* environment. But, there is no FFI module table anywhere to<br>
- * anchor the `ffi.gc` itself, and the `lua_State` object was<br>
- * marked before the function is placed on it. Hence, after the<br>
+ * anchor the `ffi.gc` itself, and the `lua_State` object is<br>
+ * marked after the function is removed from it. Hence, after the<br>
* atomic phase, the table is considered dead and collected.
Since<br>
* the table is collected, the usage of its nodes in the<br>
* `lj_gc_finalize_cdata` leads to heap-use-after-free.<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZroO7BXtZTer6kka@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Why do we need to omit the ending zero byte?
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Please add a comment that the terminating '\0' is considered by parser
as part of the input, so we must chomp it.
</pre>
</blockquote>
<p>Added:</p>
<p><br>
</p>
<p>@@ -43,6 +43,10 @@ static int unmarked_finalizer_tab_gcstart(void
*test_state)<br>
/* Not trigger GC during `lua_openffi()`. */<br>
lua_gc(L, LUA_GCSTOP, 0);<br>
<br>
+ /*<br>
+ * The terminating '\0' is considered by parser as part of<br>
+ * the input, so we must chomp it.<br>
+ */<br>
int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,<br>
"test_chunk", "t");<br>
if (res != LUA_OK) {<br>
</p>
<blockquote type="cite" cite="mid:ZroO7BXtZTer6kka@root">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">diff --git a/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua b/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua
new file mode 100644
index 00000000..fca5ec76
--- /dev/null
+++ b/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">2.34.1
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>