Re: [PATCH 45/60] ui/vnc: fix vnc_display_init() leak on failure

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Mar 17, 2026 at 12:50:59PM +0400, Marc-André Lureau wrote:
> Do not add the display state to the vnc list, if the initialization
> failed. Add vnc_display_free(), to free the display state and associated
> data in such case. The function is meant to be public and reused in the
> following changes.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/keymaps.h |  1 +
>  ui/keymaps.c | 13 ++++++++++---
>  ui/vnc.c     | 30 ++++++++++++++++++++++++++----
>  3 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/ui/keymaps.h b/ui/keymaps.h
> index 3d52c0882a1..e8917e56404 100644
> --- a/ui/keymaps.h
> +++ b/ui/keymaps.h
> @@ -54,6 +54,7 @@ typedef struct kbd_layout_t kbd_layout_t;
>  
>  kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
>                                     const char *language, Error **errp);
> +void kbd_layout_free(kbd_layout_t *k);
>  int keysym2scancode(kbd_layout_t *k, int keysym,
>                      QKbdState *kbd, bool down);
>  int keycode_is_keypad(kbd_layout_t *k, int keycode);
> diff --git a/ui/keymaps.c b/ui/keymaps.c
> index 2359dbfe7e6..d1b3f43dc8a 100644
> --- a/ui/keymaps.c
> +++ b/ui/keymaps.c
> @@ -178,6 +178,14 @@ out:
>      return ret;
>  }
>  
> +void kbd_layout_free(kbd_layout_t *k)
> +{
> +    if (!k) {
> +        return;
> +    }
> +    g_hash_table_unref(k->hash);
> +    g_free(k);
> +}
>  
>  kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
>                                     const char *language, Error **errp)
> @@ -185,10 +193,9 @@ kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
>      kbd_layout_t *k;
>  
>      k = g_new0(kbd_layout_t, 1);
> -    k->hash = g_hash_table_new(NULL, NULL);
> +    k->hash = g_hash_table_new_full(NULL, NULL, NULL, g_free);
>      if (parse_keyboard_layout(k, table, language, errp) < 0) {
> -        g_hash_table_unref(k->hash);
> -        g_free(k);
> +        kbd_layout_free(k);
>          return NULL;
>      }
>      return k;

This is fixing a memory leak in init_keyboard_layout that's separate
from the VNC leak, so these ui/keymaps.c should be their own commit.


> diff --git a/ui/vnc.c b/ui/vnc.c
> index 763b13acbde..115ff8a988e 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3421,6 +3421,8 @@ static void vmstate_change_handler(void *opaque, bool running, RunState state)
>      update_displaychangelistener(&vd->dcl, VNC_REFRESH_INTERVAL_BASE);
>  }
>  
> +static void vnc_display_free(VncDisplay *vd);
> +
>  void vnc_display_init(const char *id, Error **errp)
>  {
>      VncDisplay *vd;
> @@ -3430,8 +3432,9 @@ void vnc_display_init(const char *id, Error **errp)
>      }
>      vd = g_malloc0(sizeof(*vd));
>  
> +    qemu_mutex_init(&vd->mutex);
>      vd->id = g_strdup(id);
> -    QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
> +    vd->dcl.ops = &dcl_ops;
>  
>      QTAILQ_INIT(&vd->clients);
>      vd->expires = TIME_MAX;
> @@ -3445,22 +3448,22 @@ void vnc_display_init(const char *id, Error **errp)
>      }
>  
>      if (!vd->kbd_layout) {
> +        vnc_display_free(vd);
>          return;
>      }
>  
>      vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
>      vd->connections_limit = 32;
>  
> -    qemu_mutex_init(&vd->mutex);
>      vnc_start_worker_thread();
>  
> -    vd->dcl.ops = &dcl_ops;
>      register_displaychangelistener(&vd->dcl);
>      vd->kbd = qkbd_state_init(vd->dcl.con);
>      vd->vmstate_handler_entry = qemu_add_vm_change_state_handler(
>          &vmstate_change_handler, vd);
> -}
>  
> +    QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
> +}
>  
>  static void vnc_display_close(VncDisplay *vd)
>  {
> @@ -3504,6 +3507,25 @@ static void vnc_display_close(VncDisplay *vd)
>  #endif
>  }
>  
> +static void vnc_display_free(VncDisplay *vd)
> +{
> +    if (!vd) {
> +        return;
> +    }
> +    vnc_display_close(vd);
> +    unregister_displaychangelistener(&vd->dcl);
> +    qkbd_state_free(vd->kbd);
> +    qemu_del_vm_change_state_handler(vd->vmstate_handler_entry);
> +    kbd_layout_free(vd->kbd_layout);
> +    qemu_mutex_destroy(&vd->mutex);
> +    if (QTAILQ_IN_USE(vd, next)) {
> +        QTAILQ_REMOVE(&vnc_displays, vd, next);
> +    }
> +    g_free(vd->id);
> +    g_free(vd);
> +}

If we're introducing this we need to answer the earlier questions
in this series about killing off the VNC worker thread, as IMHO,
we should not leave the thread running if we're claiming to be
able to free VncDisplay state.

> +
> +
>  int vnc_display_password(const char *id, const char *password, Error **errp)
>  {
>      VncDisplay *vd = vnc_display_find(id);
> 
> -- 
> 2.53.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|