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

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

Hi, Sergey!
Thanks for the review!
Please, consider my answers below.

On 29.11.23, Sergey Bronnikov wrote:
> Hello, Sergey
> 
> thanks for the patch! See comments below
> 
> On 10/23/23 12:22, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Reported by elmknot.
> >
> > (cherry-picked from commit cc96ab9d513582703f8663a8775a935b56db32b7)
> >
> > During the recording of an arithmetic operation with a cdata function
> > object and some cdata value in `recff_cdata_arith()`, reallocation of
> > `cts->tab` in `lj_ctype_intern()` may occur. In that case, the reference
> > to the first `CType` object (`s[0]`) becomes invalid. This patch saves
> > the `CTypeID` of this object and gets its `CType` again after possible
> > reallocation.
> >
> > Also, this commit fills `cts->tab` memory with zeros before being freed
> > when `-DLUAJIT_CTYPE_CHECK_ANCHOR` is defined. It helps to observe
> > assertion failures in case this memory is used after free.
> 
> failed to reproduce a bug with reverted fix and LuaJIT built with a flag:
> 
> $ CMAKE_C_FLAGS=-DLUAJIT_CTYPE_CHECK_ANCHOR 
> CFLAGS=-DLUAJIT_CTYPE_CHECK_ANCHOR cmake -S . -B build 
> -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUAJIT_USE_ASAN=ON

I suppose you forgot the -DLUAJIT_USE_SYSMALLOC=ON
-DLUAJIT_ENABLE_GC64=ON flags. The same is valid for the previous patch.

> $ ./build/src/luajit 
> test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> TAP version 13
> 1..1
> ok - no heap-use-after-free in recff_cdata_arith
> 
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9145
> > ---
> >   src/lj_crecord.c                              |  4 +
> >   src/lj_ctype.c                                | 12 +++
> >   ...0-fix-dangling-reference-to-ctype.test.lua | 77 +++++++++++++++++++
> >   3 files changed, 93 insertions(+)
> >   create mode 100644 test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> >

<snipped>

> > +++ b/test/tarantool-tests/lj-920-fix-dangling-reference-to-ctype.test.lua
> > @@ -0,0 +1,77 @@
> > +local tap = require('tap')
> > +local ffi = require('ffi')
> > +local test = tap.test('lj-920-fix-dangling-reference-to-ctype'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +  -- luacheck: no global
> > +  ['Impossible to predict the value of cts->top'] = _TARANTOOL,
> 
> Let's declare _TARANTOOL as global in .luacheckrc instead of suppressing 
> warning inline every time.
> 
> About 7 tests suppress it.

I suppose it is a good thing to change, but in a separate patchset.
Ignoring for now, you may create a ticket for refactoring if you want
(to prevent falling off the edge of the earth).

> 
> > +})

<snipped>

> > +test:done(true)

-- 
Best regards,
Sergey Kaplun