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

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

Maxim,

On 12.03.24, Maxim Kokryashkin wrote:
> 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.

It is already mentioned at the beggining of the file [1].

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

<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.

Sure, see the iterative patch below. Branch is force-pushed.

===================================================================
diff --git a/test/tarantool-tests/lj-1172-debug-handling-ref.test.lua b/test/tarantool-tests/lj-1172-debug-handling-ref.test.lua
index cac1c223..cf892011 100644
--- a/test/tarantool-tests/lj-1172-debug-handling-ref.test.lua
+++ b/test/tarantool-tests/lj-1172-debug-handling-ref.test.lua
@@ -13,6 +13,8 @@ local jdump = require('jit.dump')
 
 test:plan(1)
 
+-- We need to trigger the `TRACE` vmevent handler during
+-- `debug.setmetatable()`. It will cause Lua stack reallocation.
 jdump.start('t', '/dev/null')
 
 -- Use `coroutine.wrap()` to create a new Lua stack with a minimum
===================================================================

> >
> > > > +
> > > > +-- 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)()

<snipped>

> > --
> > Best regards,
> > Sergey Kaplun

[1]: https://github.com/tarantool/luajit/blob/fead6df178f5b7a8384e217720647025eaf66e75/test/tarantool-tests/lj-1172-debug-handling-ref.test.lua#L5

-- 
Best regards,
Sergey Kaplun