<!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>fixes applied and force-pushed.</p>
    <p>Sergey<br>
    </p>
    <div class="moz-cite-prefix">On 10.07.2024 17:08, Sergey Kaplun
      wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:Zo6VyddcpyiDqmdU@root">
      <pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing minor comments below.

On 10.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 15:14, 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>
      <pre class="moz-quote-pre" wrap="">
<snipped>

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <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>
        <pre class="moz-quote-pre" wrap="">
Updated.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
| Previous patch fixes the problem partially because the introduced
| 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.

Minor: "cycle (since it isn't marked because it is not accessible from
any GC root)."</pre>
    </blockquote>
    Updated.<br>
    <blockquote type="cite" cite="mid:Zo6VyddcpyiDqmdU@root">
      <pre class="moz-quote-pre" wrap="">

|            Access to the cdata finalizer table exhibits heap use
| after free. The patch turns the finalizer table into a proper
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">
</pre>
      </blockquote>
      <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-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
</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="">+
+       /* 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>
        <pre class="moz-quote-pre" wrap="">Ok, I'll add.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Thanks!

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <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");
</pre>
          </blockquote>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
I suggest renaming "chunk" to the "test_chunk" here too.</pre>
    </blockquote>
    <p>Fixed, but after this the line becomes longer max length and I
      need to split it for two lines:<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>
      @@ -83,7 +83,8 @@ unmarked_finalizer_tab_gcmark(void *test_state)<br>
              /* Not trigger GC during `lua_openffi()`. */<br>
              lua_gc(L, LUA_GCSTOP, 0);<br>
       <br>
      -       int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,
      "chunk", "t");<br>
      +       int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,<br>
      +                                  "test_chunk", "t");<br>
              if (res != LUA_OK) {<br>
                      test_comment("error loading Lua chunk: %s",
      lua_tostring(L, -1));<br>
                      bail_out("error loading Lua chunk");</p>
    <p>I would leave "chunk" due to this. And you?</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo6VyddcpyiDqmdU@root">
      <pre class="moz-quote-pre" wrap="">

Also, please add here comment about `sizeof(buff) - 1` too.


</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+      assert_true(res == LUA_OK);
</pre>
          </blockquote>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
<snipped>

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">--- a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
+++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
@@ -78,7 +78,10 @@ unmarked_finalizer_tab_gcsweep(void *test_state)
         lua_gc(L, LUA_GCSTOP, 0);

         int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", 
"t");
-       assert_true(res == LUA_OK);
+       if (res != LUA_OK) {
+               test_comment("error loading Lua chunk: %s", 
lua_tostring(L, -1));
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Code line length is more than 80 symbols.
(Same for the previous commit.)</pre>
    </blockquote>
    Fixed for both commits.<br>
    <blockquote type="cite" cite="mid:Zo6VyddcpyiDqmdU@root">
      <pre class="moz-quote-pre" wrap="">


</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+               bail_out("error loading Lua chunk");
+       }

         /* Finish GC cycle. */
         while (!lua_gc(L, LUA_GCSTEP, -1));

</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+
+       /* Finish GC cycle. */
</pre>
          </blockquote>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Let's add "to collect the finalizer table." to be consistent with
another test.</pre>
    </blockquote>
    <p>Fixed:</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>
      @@ -93,7 +93,7 @@ unmarked_finalizer_tab_gcmark(void *test_state)<br>
                      bail_out("error loading Lua chunk");<br>
              }<br>
       <br>
      -       /* Finish GC cycle. */<br>
      +       /* Finish GC cycle to collect the finalizer table. */<br>
              while (!lua_gc(L, LUA_GCSTEP, -1));<br>
       <br>
              /* Teardown. */<br>
    </p>
    <blockquote type="cite" cite="mid:Zo6VyddcpyiDqmdU@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+      while (!lua_gc(L, LUA_GCSTEP, -1));
+
</pre>
          </blockquote>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
<snipped>

</pre>
    </blockquote>
  </body>
  <lt-container></lt-container>
</html>