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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5100 bytes --]


Hi!
Thanks for the fixes!
LGTM
 
--
Best regards,
Maxim Kokryashkin
 
  
>Среда, 25 октября 2023, 13:47 +03:00 от Sergey Kaplun <skaplun@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
 

[-- Attachment #2: Type: text/html, Size: 6406 bytes --]