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