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

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

Hi, Sergey!
Thanks for the clarifications!
See my answers below.
On Tue, Mar 12, 2024 at 08:43:44AM +0300, Sergey Kaplun wrote:
> 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].
Then add a comment mentioning that test fails only for the ASAN build.
It is quite easy to miss.
>
> >
> > 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.
Please drop a comment mentioning that.
>
> > > +
> > > +-- 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