Re: [Tarantool-patches] [PATCH luajit 4/6] FFI: Fix dangling reference to CType.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the review!
See my answers below.

On 25.10.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> On Mon, Oct 23, 2023 at 12:22:04PM +0300, Sergey Kaplun wrote:

<snipped>

> > diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
> > new file mode 100644
> > index 00000000..c0e2c07b
> > --- /dev/null
> > +++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
> > @@ -0,0 +1,59 @@
> > +local tap = require('tap')
> > +local ffi = require('ffi')
> > +local test = tap.test('fix-dangling-reference-to-ctype'):skipcond({
> > +  -- luacheck: no global
> > +  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
> > +})
> > +
> > +test:plan(1)
> > +
> > +-- This test demonstrates LuaJIT's incorrect behaviour when the
> > +-- reallocation of `cts->tab` strikes during the conversion of a
> > +-- TValue (cdata function pointer) to a C type.
> > +-- The test fails under ASAN.
> Let's change the last sentence to 'Before the patch the test fails only
> under ASAN' because now it is a bit misleading. 

Reworded, thanks!

> > +
> > +-- XXX: Just some C functions to be casted. There is no need to
> > +-- declare their prototypes correctly.
> > +ffi.cdef[[
> > +  int malloc(void);
> > +  int fprintf(void);
> > +  int printf(void);
> > +  int memset(void);
> > +  int memcpy(void);
> > +  int memmove(void);
> > +  int getppid(void);
> > +]]
> > +
> > +-- XXX: structure to set `cts->top` to 110.
> > +local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
> > +
> > +-- Anchor table to prevent cdata objects from being collected.
> > +local anchor = {}
> > +-- Each call to this function grows `cts->top` by 3.
> Please drop a comment, referring to a point in sources, so the size
> of the growth becomes obvious.

Added. See the iterative patch below.

> 
> > +local function save_new_func(func)
> > +  anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
> > +end
> > +
> > +save_new_func(ffi.C.malloc)  -- `cts->top` = 110
> > +save_new_func(ffi.C.fprintf) -- `cts->top` = 113
> > +save_new_func(ffi.C.printf)  -- `cts->top` = 116
> > +save_new_func(ffi.C.memset)  -- `cts->top` = 119
> > +save_new_func(ffi.C.memcpy)  -- `cts->top` = 122
> 
> Is it possible to bring us to this value of `cts->top`
> with a structure?

I haven't tried it, but this structure will be too big and hardly
maintained, so I prefer the following way. I suppose that there is no
need to comment this part, so the only comment I left is about the first
alignment.

> > +
> > +-- Assertions to check the `cts->top` value and step between
> > +-- calls.
> > +assert(ffi.typeinfo(122), 'cts->top >= 122')
> > +assert(not ffi.typeinfo(123), 'cts->top < 123')
> > +
> > +save_new_func(ffi.C.memmove) -- `cts->top` = 125
> > +
> > +assert(ffi.typeinfo(125), 'cts->top >= 125')
> > +assert(not ffi.typeinfo(126), 'cts->top < 126')
> > +
> > +-- Last call to grow `cts->top` up to 128, so this causes
> > +-- `cts->tab` reallocation.
> > +save_new_func(ffi.C.getppid) -- `cts->top` = 128
> 
> Should we add an extra assertion after reallocation?

Ignored, as you mentioned in the second letter.

> > +
> > +test:ok(true, 'no heap-use-after-free in lj_cconv_ct_tv')
> > +
> > +test:done(true)
> > -- 
> > 2.42.0
> > 

Branch is force pushed:
===================================================================
diff --git a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
index c0e2c07b..2ced5779 100644
--- a/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
+++ b/test/tarantool-tests/fix-dangling-reference-to-ctype.test.lua
@@ -10,7 +10,7 @@ test:plan(1)
 -- This test demonstrates LuaJIT's incorrect behaviour when the
 -- reallocation of `cts->tab` strikes during the conversion of a
 -- TValue (cdata function pointer) to a C type.
--- The test fails under ASAN.
+-- Before the patch, the test failed only under ASAN.
 
 -- XXX: Just some C functions to be casted. There is no need to
 -- declare their prototypes correctly.
@@ -30,6 +30,9 @@ local _ = ffi.new('struct {int a; long b; float c; double d;}', 0)
 -- Anchor table to prevent cdata objects from being collected.
 local anchor = {}
 -- Each call to this function grows `cts->top` by 3.
+-- `lj_ctype_new()` and `lj_ctype_intern()` during the parsing of
+-- the `CType` declaration in the `ffi.cast()` plus
+-- `lj_ctype_intern()` during the conversion to another `CType`.
 local function save_new_func(func)
   anchor[#anchor + 1] = ffi.cast('void (*)(void)', func)
 end
===================================================================

-- 
Best regards,
Sergey Kaplun