Re: [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable().

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

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

On 11.03.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> 
> The test passes before the patch, as can be seen in CI for this branch:
> https://github.com/tarantool/luajit/tree/mkokryashkin/test

I see quite the opposite [1][2].

> 
> On Mon, Mar 11, 2024 at 01:37:01PM +0300, Sergey Kaplun wrote:

<snipped>

> > ---
> >
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1172-debug-handling-ref
> > Tarantool PR: https://github.com/tarantool/tarantool/pull/9786
> > Related issues:
> > * https://github.com/tarantool/tarantool/issues/9595
> > * https://github.com/LuaJIT/LuaJIT/issues/1172
> >
> >
> >  src/lj_api.c                                  |  1 +
> >  .../lj-1172-debug-handling-ref.test.lua       | 30 +++++++++++++++++++
> >  2 files changed, 31 insertions(+)
> >  create mode 100644 test/tarantool-tests/lj-1172-debug-handling-ref.test.lua
> >
> > diff --git a/src/lj_api.c b/src/lj_api.c
> > index 3bacad33..2e915306 100644
> > --- a/src/lj_api.c
> > +++ b/src/lj_api.c

<snipped>

> > diff --git a/test/tarantool-tests/lj-1172-debug-handling-ref.test.lua b/test/tarantool-tests/lj-1172-debug-handling-ref.test.lua
> > new file mode 100644
> > index 00000000..cac1c223

<snipped>

> > +local jdump = require('jit.dump')
> > +
> > +test:plan(1)
> > +
> > +jdump.start('t', '/dev/null')
> Why do we need that call here?

Because we need to trigger the VM event, see the comment below.

> > +
> > +-- Use `coroutine.wrap()` to create a new Lua stack with a minimum
> > +-- number of stack slots.
> > +coroutine.wrap(function()
> > +  -- "TRACE flush" event handler causes stack reallocation and
> How is flush event caused?

By the `jit.dump()` as most VM events.

> > +  -- leads to heap-use-after-free. This event handler is called
> > +  -- because all traces are specialized to base metatables, so
> > +  -- if we update any base metatable, we must flush all traces.
> > +  debug.setmetatable(1, {})
> > +end)()
> > +
> > +test:ok(true, 'no heap-use-after-free error')
> > +
> > +test:done(true)
> > --
> > 2.44.0
> >

[1]: https://github.com/tarantool/luajit/actions/runs/8239484621/job/22532793546#step:6:1344
[2]: https://github.com/tarantool/luajit/actions/runs/8239484621/job/22532793913#step:6:1363

-- 
Best regards,
Sergey Kaplun