Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable().
@ 2024-03-11 10:37 Sergey Kaplun via Tarantool-patches
  2024-03-11 20:50 ` Maxim Kokryashkin via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-03-11 10:37 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable().
  2024-03-11 10:37 [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable() Sergey Kaplun via Tarantool-patches
@ 2024-03-11 20:50 ` Maxim Kokryashkin via Tarantool-patches
  2024-03-12  5:43   ` Sergey Kaplun via Tarantool-patches
  2024-04-04  9:38 ` Sergey Bronnikov via Tarantool-patches
  2024-04-11 17:02 ` Sergey Kaplun via Tarantool-patches
  2 siblings, 1 reply; 8+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-03-11 20:50 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable().
  2024-03-11 20:50 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-03-12  5:43   ` Sergey Kaplun via Tarantool-patches
  2024-03-12 11:58     ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 1 reply; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-03-12  5:43 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable().
  2024-03-12  5:43   ` Sergey Kaplun via Tarantool-patches
@ 2024-03-12 11:58     ` Maxim Kokryashkin via Tarantool-patches
  2024-03-13  7:46       ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 8+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-03-12 11:58 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable().
  2024-03-12 11:58     ` Maxim Kokryashkin via Tarantool-patches
@ 2024-03-13  7:46       ` Sergey Kaplun via Tarantool-patches
  2024-03-13  8:49         ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 1 reply; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-03-13  7:46 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable().
  2024-03-13  7:46       ` Sergey Kaplun via Tarantool-patches
@ 2024-03-13  8:49         ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 0 replies; 8+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-03-13  8:49 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable().
  2024-03-11 10:37 [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable() Sergey Kaplun via Tarantool-patches
  2024-03-11 20:50 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-04-04  9:38 ` Sergey Bronnikov via Tarantool-patches
  2024-04-11 17:02 ` Sergey Kaplun via Tarantool-patches
  2 siblings, 0 replies; 8+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-04-04  9:38 UTC (permalink / raw)
  To: Sergey Kaplun, Maxim Kokryashkin; +Cc: tarantool-patches

Hi, Sergey!

thanks for the patch! LGTM

On 3/11/24 13:37, 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')
> +
> +-- 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
> +  -- 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)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable().
  2024-03-11 10:37 [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable() Sergey Kaplun via Tarantool-patches
  2024-03-11 20:50 ` Maxim Kokryashkin via Tarantool-patches
  2024-04-04  9:38 ` Sergey Bronnikov via Tarantool-patches
@ 2024-04-11 17:02 ` Sergey Kaplun via Tarantool-patches
  2 siblings, 0 replies; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-11 17:02 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.0 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/9938
[2]: https://github.com/tarantool/tarantool/pull/9939
[3]: https://github.com/tarantool/tarantool/pull/9940

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-04-11 17:06 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-11 10:37 [Tarantool-patches] [PATCH luajit] Handle stack reallocation in debug.setmetatable() and lua_setmetatable() Sergey Kaplun via Tarantool-patches
2024-03-11 20:50 ` Maxim Kokryashkin via Tarantool-patches
2024-03-12  5:43   ` Sergey Kaplun via Tarantool-patches
2024-03-12 11:58     ` Maxim Kokryashkin via Tarantool-patches
2024-03-13  7:46       ` Sergey Kaplun via Tarantool-patches
2024-03-13  8:49         ` Maxim Kokryashkin via Tarantool-patches
2024-04-04  9:38 ` Sergey Bronnikov via Tarantool-patches
2024-04-11 17:02 ` Sergey Kaplun via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox