<!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,</p>
<p>please see comments below. Fixes applied and force-pushed.<br>
</p>
<p>Sergey<br>
</p>
<div class="moz-cite-prefix">On 10.07.2024 16:13, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:Zo6I8p-tyeAZQX2w@root">
<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>
Thanks to Sergey Bronnikov.
(cherry picked from commit dda1ac273ad946387088d91039a8ae319359903d)
There is a table `CTState->finalizer` that contains cdata finalizers.
This table is created on initialization of the `ffi` module
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">I suppose we may drop the first sentence and start like the following:
| The finalizers table is created...
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Updated.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">by calling the functions `luaopen_ffi` and `ffi_finalizer`. In some
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">I suggest the following rewording:
| by calling the `ffi_finalizer()` routine in the `luaopen_ffi()`
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Updated.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
| The finalizers table is created on initialization of the `ffi`
| module by calling the `ffi_finalizer()` routine in the
| `luaopen_ffi()`.
Here it is good to say that usually `ffi.gc()` is anchored somewhere on
the stack via the ffi library, so the finalizer table is anchored as
well.
| But, there is no FFI module table anywhere to
Minor: s/But,/If/ [*]</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Zo6I8p-tyeAZQX2w@root">
<pre class="moz-quote-pre" wrap="">
| anchor the `ffi.gc` itself, and the `lua_State` object was marked
Typo: s/,//</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:Zo6I8p-tyeAZQX2w@root">
<pre class="moz-quote-pre" wrap="">
| before the function is placed on it. Hence, after the atomic
[*] s/./, then the finalier table isn't marked./
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).
Also, we can say `lua_State` is marked when `ffi.gc()` is not on it.
| phase, the table is considered dead and collected. Since the table
| is collected, the usage of its nodes in the `lj_gc_finalize_cdata`
| leads to heap-use-after-free.
</pre>
</blockquote>
Updated.<br>
<blockquote type="cite" cite="mid:Zo6I8p-tyeAZQX2w@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">circumstances, this table could be collected by GC and then accessed by
the function `lj_gc_finalize_cdata`. This leads to a heap-use-after-free
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Please describe more verbosely why this table isn't marked and has
become garbage collected. How is it marked before the patch?
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">problem. The patch fixes the problem.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">How does the patch fix the problem?
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>
Added.<br>
<blockquote type="cite" cite="mid:Zo6I8p-tyeAZQX2w@root">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Please, add its description to the commit message too.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<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">
<pre class="moz-quote-pre" wrap="">
</pre>
<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 type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ * placed on it. Hence, after the atomic phase, the table
</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="">+{
+ /* Shared Lua state is not needed. */
+ (void)test_state;
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<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>
<pre class="moz-quote-pre" wrap="">
I see no related comment on the branch.
<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
@@ -0,0 +1,18 @@
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+-- or removing some of the functionality of it and then calls
+-- `collectgarbage`.
+-- Seehttps://github.com/LuaJIT/LuaJIT/issues/1168 for details.
+local test = tap.test('lj-1168-heap-use-after-free-on-access-to-CTState-finalizer')
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Code line is longer than 80 symbols.
Don't to update this testname after renaming of the file.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Updated.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
| +-- This test demonstrates LuaJIT's heap-use-after-free on
| +-- on cleaning of resources during shoutdown. Test simulates
Typo: s/on//
Typo: s/Test/The test/</pre>
</blockquote>
Fixed. And "shoutdown" as well was fixed.<br>
<blockquote type="cite" cite="mid:Zo6I8p-tyeAZQX2w@root">
<pre class="moz-quote-pre" wrap="">
| +-- "unloading" of the library, or removing some of the
Typo: s/the functionality of it/its functionality/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:Zo6I8p-tyeAZQX2w@root">
<pre class="moz-quote-pre" wrap="">
| +-- functionality of it and then calls `collectgarbage`.
| +-- See <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1168">https://github.com/LuaJIT/LuaJIT/issues/1168</a> for details.
</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">
<pre class="moz-quote-pre" wrap="">--
2.34.1
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>