<!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 review. Fixes applied and force-pushed.</p>
    <p>Sergey<br>
    </p>
    <p><br>
    </p>
    <div class="moz-cite-prefix">On 09.07.2024 14:52, Sergey Kaplun via
      Tarantool-patches wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <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>
    <p>Updated.</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <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>
    <p>Updated.</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <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 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>
      <pre class="moz-quote-pre" wrap="">
The filenames are too long for my taste. I suggest the following names:
<lj-1168-unmarked-finalizer-tab.test.c>
<lj-1168-unmarked-finalizer-tab.test.lua></pre>
    </blockquote>
    Renamed.<br>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">
diff --git a/src/lj_gc.c b/src/lj_gc.c
index 591862b3..42348a34 100644
--- a/src/lj_gc.c
+++ b/src/lj_gc.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-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
new file mode 100644
index 00000000..c388c6a7
--- /dev/null
+++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
@@ -0,0 +1,66 @@
+#include <string.h>
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
This include is excess.</pre>
    </blockquote>
    <p>Removed.</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+
+#include "lua.h"
+#include "lauxlib.h"
+
+#include "test.h"
+
+/*
+ * This test demonstrates LuaJIT's incorrect behaviour on
+ * loading Lua chunk.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Minor: chunk with cdata numbers.</pre>
    </blockquote>
    <p>Updated.</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+ * 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.
+ *
+ * The GC is driving forward during parsing of the Lua chunk (`chunk`).
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Comment line is more than 66 symbols. Here and below.</pre>
    </blockquote>
    <p>Updated.</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+ * The chunk contains plenty of global objects, and the parsing
+ * of this creates a lot of strings to be used as keys in `_G`.
+ * Also, it contains several imaginary numbers (1i, 4i, 8i).
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
This chunk doesn't contains such objects, only cdata number 1LL.</pre>
    </blockquote>
    Right, updated.<br>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+ * That leads to the opening of the FFI library on-demand during the
+ * parsing of these numbers. After the FFI library is open, `ffi.gc`
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
(this number).</pre>
    </blockquote>
    <p>Fixed.</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <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
+ * placed on it. Hence, after the atomic 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>
      <pre class="moz-quote-pre" wrap="">
It will be nice to add this comment to the commit message.</pre>
    </blockquote>
    Added.<br>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+ */
+
+const char buff[] = "return 1LL";
+
+/*
+ * lua_close is a part of testcase, so testcase creates
+ * it's own Lua state and closes it at the end.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Typo: s/it's/its/</pre>
    </blockquote>
    <p>Fixed.</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+ */
+static int unmarked_finalizer_tab_gcstart(void *test_state)
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">

Minor: Maybe rename this variable like the following:
test_state -> unused;
</pre>
    </blockquote>
    <p>Renamed.</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+{
+       /* Shared Lua state is not needed. */
+       (void)test_state;
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Maybe it is better to introduce the UNUSED macro in the <utils.h> for
it. We already defined such macro for the following tests:

* fix-yield-c-hook.test.c
* lj-549-lua-load.test.c
* misclib-sysprof-capi.test.c
* unit-tap.test.c</pre>
    </blockquote>
    <p>Added a macro UNUSED and renamed unused to test_data back,</p>
    <p>because `UNUSED(unused)` doesn't look very good (because масло
      масляное).<br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+
+       /* Setup. */
+       lua_State *L = luaL_newstate();
+
+       /* Set GC at the start. */
+       lua_gc(L, LUA_GCCOLLECT, 0);
+
+       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?

Maybe name "test_chunk" is better? Feel free to ignore.</pre>
    </blockquote>
    Renamed.<br>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+          printf("Err: %s\n", lua_tostring(L, -1));
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Please use bail_out instead of `printf()`.</pre>
    </blockquote>
    <p>Fixed.</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

| test_comment("error running payload: %s", lua_tostring(L, -1));
| bail_out("error running " __func__ " test payload");

Maybe it is worth introducing some helpers in the <utils.h> for loading
and running Lua code inside our C tests.</pre>
    </blockquote>
    <p>I don't think this code is complicated enough to introduce a
      helper for it.</p>
    <p>Ignored.<br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+
+       /* Finish GC cycle. */
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Minor: s/./ to collect the finalizer table./</pre>
    </blockquote>
    <p>Updated.</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+  while (!lua_gc(L, LUA_GCSTEP, -1));
+
+       /* Teardown. */
+       lua_settop(L, 0);
+       lua_close(L);
+
+       return TEST_EXIT_SUCCESS;
+}
+
+int main(void)
+{
+       const struct test_unit tgroup[] = {
+               test_unit_def(unmarked_finalizer_tab_gcstart),
+       };
+       const int test_result = test_run_group(tgroup, NULL);
+
+       return test_result;
+}
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 @@
+local tap = require('tap')
+
+-- This test demonstrates LuaJIT's heap-use-after-free
+-- on collecting garbage. Test simulates "unloading" of the library,
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
I'd rather say that "on cleaning of resources during shoutdown", since
it happens during `lua_close()`.</pre>
    </blockquote>
    Updated.<br>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

Comment width is more than 66 lines.

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+-- or removing some of the 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.
+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>
    Updated.<br>
    <blockquote type="cite" cite="mid:Zo0kYDoAdWYUdnw_@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+test:plan(1)
+
+local ffi = require('ffi')
+
+ffi.gc = nil
+collectgarbage()
+
+test:ok(true, 'no heap use after free')
+
+test:done(true)
-- 
2.34.1

</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
</pre>
    </blockquote>
  </body>
  <lt-container></lt-container>
</html>