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

On Mon, Mar 11, 2024 at 01:37:01PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun.
>
> (cherry picked from commit 88ed9fdbbba632d174a473a0a97c914089c2916d)
>
> When we use the aforementioned functions to set a metatable for types
> with one shared metatable, we must flush all traces since they are
> specialized to base metatables. If we have enabled vmevent handlers,
> they invoke a callback on trace flushing. This callback may reallocate
> the Lua stack. Thus invalidates the reference to the `TValue *` object
> `o` by the given index in the `lua_setmetatable()` and leads to a
> heap-use-after-free error.
>
> This patch fixes the behaviour by recalculating the address by the given
> index after possible stack reallocation.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9595
> ---
>
> 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
> @@ -1067,6 +1067,7 @@ LUA_API int lua_setmetatable(lua_State *L, int idx)
>      /* Flush cache, since traces specialize to basemt. But not during __gc. */
>      if (lj_trace_flushall(L))
>        lj_err_caller(L, LJ_ERR_NOGCMM);
> +    o = index2adr(L, idx);  /* Stack may have been reallocated. */
>      if (tvisbool(o)) {
>        /* NOBARRIER: basemt is a GC root. */
>        setgcref(basemt_it(g, LJ_TTRUE), obj2gco(mt));
> 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
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1172-debug-handling-ref.test.lua
> @@ -0,0 +1,30 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate the heap-use-after-free, error for
> +-- `debug.setmetatable()` and enabled `jit.dump()`.
> +-- The test fails under ASAN.
> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1172.
> +
> +local test = tap.test('lj-1172-debug-handling-ref'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +local jdump = require('jit.dump')
> +
> +test:plan(1)
> +
> +jdump.start('t', '/dev/null')
Why do we need that call here?
> +
> +-- 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?
> +  -- 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
>