<HTML><BODY><div>Hi!</div><div>Thanks for the fixes!</div><div>LGTM</div><div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Среда, 25 октября 2023, 13:47 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16982308611606631680_BODY">Hi, Maxim!<br>Thanks for the review!<br>See my answers below.<br><br>On 25.10.23, Maxim Kokryashkin wrote:<br>> Hi, Sergey!<br>> Thanks for the patch!<br>> Please consider my comments below.<br>> On Mon, Oct 23, 2023 at 12:22:04PM +0300, Sergey Kaplun wrote:<br><br><snipped><br><br>> > diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua<br>> > new file mode 100644<br>> > index 00000000..c0e2c07b<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua<br>> > @@ -0,0 +1,59 @@<br>> > +local tap = require('tap')<br>> > +local ffi = require('ffi')<br>> > +local test = tap.test('fix-dangling-reference-to-ctype'):skipcond({<br>> > + -- luacheck: no global<br>> > + ['Impossible to predict the value of cts->top'] = _TARANTOOL,<br>> > +})<br>> > +<br>> > +test:plan(1)<br>> > +<br>> > +-- This test demonstrates LuaJIT's incorrect behaviour when the<br>> > +-- reallocation of `cts->tab` strikes during the conversion of a<br>> > +-- TValue (cdata function pointer) to a C type.<br>> > +-- The test fails under ASAN.<br>> Let's change the last sentence to 'Before the patch the test fails only<br>> under ASAN' because now it is a bit misleading.<br><br>Reworded, thanks!<br><br>> > +<br>> > +-- XXX: Just some C functions to be casted. There is no need to<br>> > +-- declare their prototypes correctly.<br>> > +ffi.cdef[[<br>> > + int malloc(void);<br>> > + int fprintf(void);<br>> > + int printf(void);<br>> > + int memset(void);<br>> > + int memcpy(void);<br>> > + int memmove(void);<br>> > + int getppid(void);<br>> > +]]<br>> > +<br>> > +-- XXX: structure to set `cts->top` to 110.<br>> > +local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)<br>> > +<br>> > +-- Anchor table to prevent cdata objects from being collected.<br>> > +local anchor = {}<br>> > +-- Each call to this function grows `cts->top` by 3.<br>> Please drop a comment, referring to a point in sources, so the size<br>> of the growth becomes obvious.<br><br>Added. See the iterative patch below.<br><br>><br>> > +local function save_new_func(func)<br>> > + anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)<br>> > +end<br>> > +<br>> > +save_new_func(ffi.C.malloc) -- `cts->top` = 110<br>> > +save_new_func(ffi.C.fprintf) -- `cts->top` = 113<br>> > +save_new_func(ffi.C.printf) -- `cts->top` = 116<br>> > +save_new_func(ffi.C.memset) -- `cts->top` = 119<br>> > +save_new_func(ffi.C.memcpy) -- `cts->top` = 122<br>><br>> Is it possible to bring us to this value of `cts->top`<br>> with a structure?<br><br>I haven't tried it, but this structure will be too big and hardly<br>maintained, so I prefer the following way. I suppose that there is no<br>need to comment this part, so the only comment I left is about the first<br>alignment.<br><br>> > +<br>> > +-- Assertions to check the `cts->top` value and step between<br>> > +-- calls.<br>> > +assert(ffi.typeinfo(122), 'cts->top >= 122')<br>> > +assert(not ffi.typeinfo(123), 'cts->top < 123')<br>> > +<br>> > +save_new_func(ffi.C.memmove) -- `cts->top` = 125<br>> > +<br>> > +assert(ffi.typeinfo(125), 'cts->top >= 125')<br>> > +assert(not ffi.typeinfo(126), 'cts->top < 126')<br>> > +<br>> > +-- Last call to grow `cts->top` up to 128, so this causes<br>> > +-- `cts->tab` reallocation.<br>> > +save_new_func(ffi.C.getppid) -- `cts->top` = 128<br>><br>> Should we add an extra assertion after reallocation?<br><br>Ignored, as you mentioned in the second letter.<br><br>> > +<br>> > +test:ok(true, 'no heap-use-after-free in lj_cconv_ct_tv')<br>> > +<br>> > +test:done(true)<br>> > --<br>> > 2.42.0<br>> ><br><br>Branch is force pushed:<br>===================================================================<br>diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua<br>index c0e2c07b..2ced5779 100644<br>--- a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua<br>+++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua<br>@@ -10,7 +10,7 @@ test:plan(1)<br> -- This test demonstrates LuaJIT's incorrect behaviour when the<br> -- reallocation of `cts->tab` strikes during the conversion of a<br> -- TValue (cdata function pointer) to a C type.<br>--- The test fails under ASAN.<br>+-- Before the patch, the test failed only under ASAN.<br> <br> -- XXX: Just some C functions to be casted. There is no need to<br> -- declare their prototypes correctly.<br>@@ -30,6 +30,9 @@ local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)<br> -- Anchor table to prevent cdata objects from being collected.<br> local anchor = {}<br> -- Each call to this function grows `cts->top` by 3.<br>+-- `lj_ctype_new()` and `lj_ctype_intern()` during the parsing of<br>+-- the `CType` declaration in the `ffi.cast()` plus<br>+-- `lj_ctype_intern()` during the conversion to another `CType`.<br> local function save_new_func(func)<br> anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)<br> end<br>===================================================================<br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></BODY></HTML>