<!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 15:14, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:Zo0pr5uIh3n-VZUx@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>
Reported by Sergey Bronnikov.
(cherry picked from commit f5affaa6c4e7524e661484f22f24255f9a83eb47)
Previous patch fixes the problem partially because the introduced
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/Previous/The previous/
Typo: s/fixes (.*) partially/partially fixes \1/</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Zo0pr5uIh3n-VZUx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">GC root may not exist at the start phase of the GC cycle. In that
case, the cdata finalizer table will be collected at the end of
the cycle. Access to the cdata finalizer table exhibits heap use
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: "will be collected at the end of the cycle if it is created after
the start phase."</pre>
</blockquote>
<p>Updated.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Zo0pr5uIh3n-VZUx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">after free. The patch is turned the finalizer table into a proper
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/is turned/turns/
</pre>
</blockquote>
<p>Updated.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Zo0pr5uIh3n-VZUx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">GC root.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
It is worth mentioning that this table is created on the initialization
of the main Lua State instead of loading the FFI library.
</pre>
</blockquote>
Added.<br>
<blockquote type="cite" cite="mid:Zo0pr5uIh3n-VZUx@root">
<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/lib_ffi.c | 20 +--------
src/lj_cdata.c | 2 +-
src/lj_ctype.c | 12 ++++++
src/lj_ctype.h | 2 +-
src/lj_gc.c | 41 ++++++++----------
src/lj_obj.h | 3 ++
src/lj_state.c | 3 ++
...free-on-access-to-CTState-finalizer.test.c | 42 +++++++++++++++++++
8 files changed, 81 insertions(+), 44 deletions(-)
diff --git a/src/lib_ffi.c b/src/lib_ffi.c
index 7ed6fc78..3c8dd77f 100644
--- a/src/lib_ffi.c
+++ b/src/lib_ffi.c
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">diff --git a/src/lj_cdata.c b/src/lj_cdata.c
index 35d0e76a..3d6ff1cc 100644
--- a/src/lj_cdata.c
+++ b/src/lj_cdata.c
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">diff --git a/src/lj_ctype.c b/src/lj_ctype.c
index 53b83031..c0213629 100644
--- a/src/lj_ctype.c
+++ b/src/lj_ctype.c
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">diff --git a/src/lj_ctype.h b/src/lj_ctype.h
index 8edbd561..2d393eb9 100644
--- a/src/lj_ctype.h
+++ b/src/lj_ctype.h
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">diff --git a/src/lj_gc.c b/src/lj_gc.c
index 42348a34..4c222f21 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/src/lj_obj.h b/src/lj_obj.h
index 69e94ff2..06ea0cd0 100644
--- a/src/lj_obj.h
+++ b/src/lj_obj.h
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">diff --git a/src/lj_state.c b/src/lj_state.c
index 01d4901a..5a920102 100644
--- a/src/lj_state.c
+++ b/src/lj_state.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
index c388c6a7..259528cb 100644
--- 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
@@ -55,10 +55,52 @@ static int unmarked_finalizer_tab_gcstart(void *test_state)
return TEST_EXIT_SUCCESS;
}
+static int
+unmarked_finalizer_tab_gcsweep(void *test_state)
+{
+ const char buff[] = "return 1LL";
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Why do we need an additional buffer if the same one already exists?</pre>
</blockquote>
Left a single buffer.<br>
<blockquote type="cite" cite="mid:Zo0pr5uIh3n-VZUx@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;
+
+ /* Setup. */
+ lua_State *L = luaL_newstate();
+
+ /* Set GC at the start. */
+ lua_gc(L, LUA_GCCOLLECT, 0);
+
+ /*
+ * Default step is too big -- one step ends after the
+ * atomic phase.
+ */
+ lua_gc(L, LUA_GCSETSTEPMUL, 1);
+
+ /* Skip marking roots. */
+ lua_gc(L, LUA_GCSTEP, 1);
+
+ /* Not trigger GC during `lua_openffi()`. */
+ lua_gc(L, LUA_GCSTOP, 0);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Maybe it is worth adding this GC stop for the first test case too to
make it more robust.</pre>
</blockquote>
Ok, I'll add.<br>
<blockquote type="cite" cite="mid:Zo0pr5uIh3n-VZUx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t");
+ assert_true(res == LUA_OK);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I suppose it is better to use not assert_true here but `test_comment()`
and `bail_out()`, since this is not behaviour that we are testing.</pre>
</blockquote>
<p>Updated:</p>
<p><br>
</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>
@@ -78,7 +78,10 @@ unmarked_finalizer_tab_gcsweep(void
*test_state)<br>
lua_gc(L, LUA_GCSTOP, 0);<br>
<br>
int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,
"chunk", "t");<br>
- assert_true(res == LUA_OK);<br>
+ if (res != LUA_OK) {<br>
+ test_comment("error loading Lua chunk: %s",
lua_tostring(L, -1));<br>
+ bail_out("error loading Lua chunk");<br>
+ }<br>
<br>
/* Finish GC cycle. */<br>
while (!lua_gc(L, LUA_GCSTEP, -1));<br>
</p>
<blockquote type="cite" cite="mid:Zo0pr5uIh3n-VZUx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ /* Finish GC cycle. */
+ while (!lua_gc(L, LUA_GCSTEP, -1));
+
+ assert_true(lua_gettop(L) == 1);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Why do we need this assert?</pre>
</blockquote>
<p>removed</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Zo0pr5uIh3n-VZUx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ /* 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),
+ test_unit_def(unmarked_finalizer_tab_gcsweep),
};
const int test_result = test_run_group(tgroup, NULL);
--
2.34.1
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>