[PATCH] ui/console: remove console from global list on finalization

[PATCH] ui/console: remove console from global list on finalization

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This commit removes the QemuConsole from the global "consoles" list when
it is finalized.

Previously, there was a TODO comment indicating this path needed
checking. The assertions added ensure that `dcls`, `gl_block`, and the
`dump_queue` are empty before removal, confirming the console is in a
clean state.

Fix potential use-after-free crashes when a display console is removed.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ui/console.c b/ui/console.c
index f445db11389..b64e2122f34 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -394,10 +394,13 @@ qemu_console_finalize(Object *obj)
 {
     QemuConsole *c = QEMU_CONSOLE(obj);
 
-    /* TODO: check this code path, and unregister from consoles */
+    assert(c->dcls == 0);
+    assert(c->gl_block == 0);
+    assert(qemu_co_queue_empty(&c->dump_queue));
     g_clear_pointer(&c->surface, qemu_free_displaysurface);
     g_clear_pointer(&c->gl_unblock_timer, timer_free);
     g_clear_pointer(&c->ui_timer, timer_free);
+    QTAILQ_REMOVE(&consoles, c, next);
 }
 
 static void
-- 
2.53.0

Re: [PATCH] ui/console: remove console from global list on finalization

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This commit removes the QemuConsole from the global "consoles" list when
> it is finalized.
>
> Previously, there was a TODO comment indicating this path needed
> checking. The assertions added ensure that `dcls`, `gl_block`, and the
> `dump_queue` are empty before removal, confirming the console is in a
> clean state.
>
> Fix potential use-after-free crashes when a display console is removed.
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/console.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/ui/console.c b/ui/console.c
> index f445db11389..b64e2122f34 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -394,10 +394,13 @@ qemu_console_finalize(Object *obj)
>  {
>      QemuConsole *c = QEMU_CONSOLE(obj);
>  
> -    /* TODO: check this code path, and unregister from consoles */
> +    assert(c->dcls == 0);
> +    assert(c->gl_block == 0);
> +    assert(qemu_co_queue_empty(&c->dump_queue));

Help me out: what ensures this?

>      g_clear_pointer(&c->surface, qemu_free_displaysurface);
>      g_clear_pointer(&c->gl_unblock_timer, timer_free);
>      g_clear_pointer(&c->ui_timer, timer_free);
> +    QTAILQ_REMOVE(&consoles, c, next);

Is @consoles only accessed from the main thread?

>  }
>  
>  static void

Re: [PATCH] ui/console: remove console from global list on finalization

[Marc-André Lureau]

Hi

On Thu, Apr 23, 2026 at 9:02 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > This commit removes the QemuConsole from the global "consoles" list when
> > it is finalized.
> >
> > Previously, there was a TODO comment indicating this path needed
> > checking. The assertions added ensure that `dcls`, `gl_block`, and the
> > `dump_queue` are empty before removal, confirming the console is in a
> > clean state.
> >
> > Fix potential use-after-free crashes when a display console is removed.
> >
> > Reported-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  ui/console.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/ui/console.c b/ui/console.c
> > index f445db11389..b64e2122f34 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -394,10 +394,13 @@ qemu_console_finalize(Object *obj)
> >  {
> >      QemuConsole *c = QEMU_CONSOLE(obj);
> >
> > -    /* TODO: check this code path, and unregister from consoles */
> > +    assert(c->dcls == 0);
> > +    assert(c->gl_block == 0);
> > +    assert(qemu_co_queue_empty(&c->dump_queue));
>
> Help me out: what ensures this?

- No display change listener left
- No GL lock left
- No pending screendump

>
> >      g_clear_pointer(&c->surface, qemu_free_displaysurface);
> >      g_clear_pointer(&c->gl_unblock_timer, timer_free);
> >      g_clear_pointer(&c->ui_timer, timer_free);
> > +    QTAILQ_REMOVE(&consoles, c, next);
>
> Is @consoles only accessed from the main thread?

Yes, the UI code is main thread only.

Re: [PATCH] ui/console: remove console from global list on finalization

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Thu, Apr 23, 2026 at 9:02 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > This commit removes the QemuConsole from the global "consoles" list when
>> > it is finalized.
>> >
>> > Previously, there was a TODO comment indicating this path needed
>> > checking. The assertions added ensure that `dcls`, `gl_block`, and the
>> > `dump_queue` are empty before removal, confirming the console is in a
>> > clean state.
>> >
>> > Fix potential use-after-free crashes when a display console is removed.
>> >
>> > Reported-by: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  ui/console.c | 5 ++++-
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/ui/console.c b/ui/console.c
>> > index f445db11389..b64e2122f34 100644
>> > --- a/ui/console.c
>> > +++ b/ui/console.c
>> > @@ -394,10 +394,13 @@ qemu_console_finalize(Object *obj)
>> >  {
>> >      QemuConsole *c = QEMU_CONSOLE(obj);
>> >
>> > -    /* TODO: check this code path, and unregister from consoles */
>> > +    assert(c->dcls == 0);
>> > +    assert(c->gl_block == 0);
>> > +    assert(qemu_co_queue_empty(&c->dump_queue));
>>
>> Help me out: what ensures this?
>
> - No display change listener left
> - No GL lock left
> - No pending screendump

Yes, but what ensures none of these are left / pending by the time we
finalize?

>> >      g_clear_pointer(&c->surface, qemu_free_displaysurface);
>> >      g_clear_pointer(&c->gl_unblock_timer, timer_free);
>> >      g_clear_pointer(&c->ui_timer, timer_free);
>> > +    QTAILQ_REMOVE(&consoles, c, next);
>>
>> Is @consoles only accessed from the main thread?
>
> Yes, the UI code is main thread only.

Good, thanks!

Re: [PATCH] ui/console: remove console from global list on finalization

[Marc-André Lureau]

Hi

On Thu, Apr 23, 2026 at 10:59 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Hi
> >
> > On Thu, Apr 23, 2026 at 9:02 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> marcandre.lureau@redhat.com writes:
> >>
> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >
> >> > This commit removes the QemuConsole from the global "consoles" list when
> >> > it is finalized.
> >> >
> >> > Previously, there was a TODO comment indicating this path needed
> >> > checking. The assertions added ensure that `dcls`, `gl_block`, and the
> >> > `dump_queue` are empty before removal, confirming the console is in a
> >> > clean state.
> >> >
> >> > Fix potential use-after-free crashes when a display console is removed.
> >> >
> >> > Reported-by: Markus Armbruster <armbru@redhat.com>
> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> > ---
> >> >  ui/console.c | 5 ++++-
> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/ui/console.c b/ui/console.c
> >> > index f445db11389..b64e2122f34 100644
> >> > --- a/ui/console.c
> >> > +++ b/ui/console.c
> >> > @@ -394,10 +394,13 @@ qemu_console_finalize(Object *obj)
> >> >  {
> >> >      QemuConsole *c = QEMU_CONSOLE(obj);
> >> >
> >> > -    /* TODO: check this code path, and unregister from consoles */
> >> > +    assert(c->dcls == 0);
> >> > +    assert(c->gl_block == 0);
> >> > +    assert(qemu_co_queue_empty(&c->dump_queue));
> >>
> >> Help me out: what ensures this?
> >
> > - No display change listener left
> > - No GL lock left
> > - No pending screendump
>
> Yes, but what ensures none of these are left / pending by the time we
> finalize?

Unfortunately, we don't have much support for unplugging display
consoles. So those asserts are mostly there to remind us of further
issues.. I should probably leave the TODO.

In general graphics devices do not support hot-plugging. It looks like
we are missing a couple of hotpluggable = false in hw/display. So, it
should not be reachable today but by using low-level QMP/QOM like in
this test.

Text console/VC is also poorly supported and leaks, so it will never
reach qemu_console_finalize() either.

I can try to improve the situation by sending a more complete series,
so those assert() are unlikely to be reachable in the future.


>
> >> >      g_clear_pointer(&c->surface, qemu_free_displaysurface);
> >> >      g_clear_pointer(&c->gl_unblock_timer, timer_free);
> >> >      g_clear_pointer(&c->ui_timer, timer_free);
> >> > +    QTAILQ_REMOVE(&consoles, c, next);
> >>
> >> Is @consoles only accessed from the main thread?
> >
> > Yes, the UI code is main thread only.
>
> Good, thanks!
>

Re: [PATCH] ui/console: remove console from global list on finalization

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Thu, Apr 23, 2026 at 10:59 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Hi
>> >
>> > On Thu, Apr 23, 2026 at 9:02 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> marcandre.lureau@redhat.com writes:
>> >>
>> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> >
>> >> > This commit removes the QemuConsole from the global "consoles" list when
>> >> > it is finalized.
>> >> >
>> >> > Previously, there was a TODO comment indicating this path needed
>> >> > checking. The assertions added ensure that `dcls`, `gl_block`, and the
>> >> > `dump_queue` are empty before removal, confirming the console is in a
>> >> > clean state.
>> >> >
>> >> > Fix potential use-after-free crashes when a display console is removed.
>> >> >
>> >> > Reported-by: Markus Armbruster <armbru@redhat.com>
>> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> > ---
>> >> >  ui/console.c | 5 ++++-
>> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/ui/console.c b/ui/console.c
>> >> > index f445db11389..b64e2122f34 100644
>> >> > --- a/ui/console.c
>> >> > +++ b/ui/console.c
>> >> > @@ -394,10 +394,13 @@ qemu_console_finalize(Object *obj)
>> >> >  {
>> >> >      QemuConsole *c = QEMU_CONSOLE(obj);
>> >> >
>> >> > -    /* TODO: check this code path, and unregister from consoles */
>> >> > +    assert(c->dcls == 0);
>> >> > +    assert(c->gl_block == 0);
>> >> > +    assert(qemu_co_queue_empty(&c->dump_queue));
>> >>
>> >> Help me out: what ensures this?
>> >
>> > - No display change listener left
>> > - No GL lock left
>> > - No pending screendump
>>
>> Yes, but what ensures none of these are left / pending by the time we
>> finalize?
>
> Unfortunately, we don't have much support for unplugging display
> consoles. So those asserts are mostly there to remind us of further
> issues.. I should probably leave the TODO.
>
> In general graphics devices do not support hot-plugging. It looks like
> we are missing a couple of hotpluggable = false in hw/display.

I trust you'll take care of them.

>                                                                So, it
> should not be reachable today but by using low-level QMP/QOM like in
> this test.

Due to QOM's design, introspection must create and destroy a temporary
object.  This must not have observable side effects.

Devices have a life cycle supporting this:

    instance_init -+-> realize ---> unrealize -+-> instance_finalize.
                   |                           |
                   +---------------------------+

We can keep instance_init and instance_finalize free of side effects by
doing them in realize and unrealize instead.

Non-device objects lack realize / unrealize.  I believe the wheel has
been reinvented a few times there.

Back to qemu_console_finalize().  I guess the correctness argument goes
roughly like this:

1. After initialization, these assertions hold.

2. Therefore, immediate finalize works.  QOM introspection works.

3. Non-immediate finalization cannot happen.

Is this about right?

> Text console/VC is also poorly supported and leaks, so it will never
> reach qemu_console_finalize() either.
>
> I can try to improve the situation by sending a more complete series,
> so those assert() are unlikely to be reachable in the future.

I'm just trying to understand why this works :)

More complete patches are always nice, but I'm not demanding you do that
now.  Comments perhaps?

>> >> >      g_clear_pointer(&c->surface, qemu_free_displaysurface);
>> >> >      g_clear_pointer(&c->gl_unblock_timer, timer_free);
>> >> >      g_clear_pointer(&c->ui_timer, timer_free);
>> >> > +    QTAILQ_REMOVE(&consoles, c, next);
>> >>
>> >> Is @consoles only accessed from the main thread?
>> >
>> > Yes, the UI code is main thread only.
>>
>> Good, thanks!
>>

Re: [PATCH] ui/console: remove console from global list on finalization

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This commit removes the QemuConsole from the global "consoles" list when
> it is finalized.
>
> Previously, there was a TODO comment indicating this path needed
> checking. The assertions added ensure that `dcls`, `gl_block`, and the
> `dump_queue` are empty before removal, confirming the console is in a
> clean state.
>
> Fix potential use-after-free crashes when a display console is removed.

Suggest to mention reproducers: QMP command qom-list-properties with
typename "qemu-text-console", "qemu-fixed-text-console" or
"qemu-graphic-console".

> Reported-by: Markus Armbruster <armbru@redhat.com>

Please add

  Cc: qemu-stable@nongnu.org

and Fixes: if it's not too much trouble.

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH] ui/console: remove console from global list on finalization

[Marc-André Lureau]

Hi

On Thu, Apr 23, 2026 at 2:59 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Hi
> >
> > On Thu, Apr 23, 2026 at 10:59 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> >>
> >> > Hi
> >> >
> >> > On Thu, Apr 23, 2026 at 9:02 AM Markus Armbruster <armbru@redhat.com> wrote:
> >> >>
> >> >> marcandre.lureau@redhat.com writes:
> >> >>
> >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> >
> >> >> > This commit removes the QemuConsole from the global "consoles" list when
> >> >> > it is finalized.
> >> >> >
> >> >> > Previously, there was a TODO comment indicating this path needed
> >> >> > checking. The assertions added ensure that `dcls`, `gl_block`, and the
> >> >> > `dump_queue` are empty before removal, confirming the console is in a
> >> >> > clean state.
> >> >> >
> >> >> > Fix potential use-after-free crashes when a display console is removed.
> >> >> >
> >> >> > Reported-by: Markus Armbruster <armbru@redhat.com>
> >> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> > ---
> >> >> >  ui/console.c | 5 ++++-
> >> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/ui/console.c b/ui/console.c
> >> >> > index f445db11389..b64e2122f34 100644
> >> >> > --- a/ui/console.c
> >> >> > +++ b/ui/console.c
> >> >> > @@ -394,10 +394,13 @@ qemu_console_finalize(Object *obj)
> >> >> >  {
> >> >> >      QemuConsole *c = QEMU_CONSOLE(obj);
> >> >> >
> >> >> > -    /* TODO: check this code path, and unregister from consoles */
> >> >> > +    assert(c->dcls == 0);
> >> >> > +    assert(c->gl_block == 0);
> >> >> > +    assert(qemu_co_queue_empty(&c->dump_queue));
> >> >>
> >> >> Help me out: what ensures this?
> >> >
> >> > - No display change listener left
> >> > - No GL lock left
> >> > - No pending screendump
> >>
> >> Yes, but what ensures none of these are left / pending by the time we
> >> finalize?
> >
> > Unfortunately, we don't have much support for unplugging display
> > consoles. So those asserts are mostly there to remind us of further
> > issues.. I should probably leave the TODO.
> >
> > In general graphics devices do not support hot-plugging. It looks like
> > we are missing a couple of hotpluggable = false in hw/display.
>
> I trust you'll take care of them.
>
> >                                                                So, it
> > should not be reachable today but by using low-level QMP/QOM like in
> > this test.
>
> Due to QOM's design, introspection must create and destroy a temporary
> object.  This must not have observable side effects.
>
> Devices have a life cycle supporting this:
>
>     instance_init -+-> realize ---> unrealize -+-> instance_finalize.
>                    |                           |
>                    +---------------------------+
>
> We can keep instance_init and instance_finalize free of side effects by
> doing them in realize and unrealize instead.
>
> Non-device objects lack realize / unrealize.  I believe the wheel has
> been reinvented a few times there.
>
> Back to qemu_console_finalize().  I guess the correctness argument goes
> roughly like this:
>
> 1. After initialization, these assertions hold.
>
> 2. Therefore, immediate finalize works.  QOM introspection works.
>
> 3. Non-immediate finalization cannot happen.
>
> Is this about right?

I am confident it can happen on hot-unplug, but it's not working proprely.

>
> > Text console/VC is also poorly supported and leaks, so it will never
> > reach qemu_console_finalize() either.
> >
> > I can try to improve the situation by sending a more complete series,
> > so those assert() are unlikely to be reachable in the future.
>
> I'm just trying to understand why this works :)
>
> More complete patches are always nice, but I'm not demanding you do that
> now.  Comments perhaps?

I think we should leave a TODO & asserts for the hot-unplug cases

Re: [PATCH] ui/console: remove console from global list on finalization

[Marc-André Lureau]

Hi

On Fri, Apr 24, 2026 at 10:52 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > This commit removes the QemuConsole from the global "consoles" list when
> > it is finalized.
> >
> > Previously, there was a TODO comment indicating this path needed
> > checking. The assertions added ensure that `dcls`, `gl_block`, and the
> > `dump_queue` are empty before removal, confirming the console is in a
> > clean state.
> >
> > Fix potential use-after-free crashes when a display console is removed.
>
> Suggest to mention reproducers: QMP command qom-list-properties with
> typename "qemu-text-console", "qemu-fixed-text-console" or
> "qemu-graphic-console".
>
> > Reported-by: Markus Armbruster <armbru@redhat.com>
>
> Please add
>
>   Cc: qemu-stable@nongnu.org
>
> and Fixes: if it's not too much trouble.
>

Given that the hot-unplug path is not tested and has probably not been
working correctly since forever, I don't think we should backport it
or look for Fixes commits.

Re: [PATCH] ui/console: remove console from global list on finalization

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Fri, Apr 24, 2026 at 10:52 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > This commit removes the QemuConsole from the global "consoles" list when
>> > it is finalized.
>> >
>> > Previously, there was a TODO comment indicating this path needed
>> > checking. The assertions added ensure that `dcls`, `gl_block`, and the
>> > `dump_queue` are empty before removal, confirming the console is in a
>> > clean state.
>> >
>> > Fix potential use-after-free crashes when a display console is removed.
>>
>> Suggest to mention reproducers: QMP command qom-list-properties with
>> typename "qemu-text-console", "qemu-fixed-text-console" or
>> "qemu-graphic-console".
>>
>> > Reported-by: Markus Armbruster <armbru@redhat.com>
>>
>> Please add
>>
>>   Cc: qemu-stable@nongnu.org
>>
>> and Fixes: if it's not too much trouble.
>>
>
> Given that the hot-unplug path is not tested and has probably not been
> working correctly since forever, I don't think we should backport it
> or look for Fixes commits.

Hmm, makes sense.  Add a suitable "careful, this is broken" comment?