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 fixes!
LGTM
On Wed, Mar 13, 2024 at 10:46:32AM +0300, Sergey Kaplun wrote:
> 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