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

[Markus Armbruster <armbru@redhat.com>]

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