Re: [libvirt] [libvirt-glib] Fix potential race in GVirObjectConnection::domain_event_cb

This method starts by looking up a domain in priv->domains, and later, depending on the kind of event that occurred, it may remove this domain from priv->domains. While the individual operations (lookup, removal) are protected by priv->lock, there is no guarantee that the looked up domain and even priv->domains will stay unchanged when priv->lock isn't held. In particular, gvir_connection_close will destroy priv->domains which will unreference all the domains it contains (potentially destroying them too), and gvir_connection_fetch_domains will change priv->domains value. To avoid these issues, this commit takes a reference on priv->domains so that it doesn't get away behind our back, and it takes a reference on the looked up domain too to ensure it stays alive for the duration of domain_event_cb run. --- libvirt-gobject/libvirt-gobject-connection.c | 17 +++++++++++------ 1 files changed, 11 insertions(+), 6 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index 59b828d..786a026 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -257,6 +257,7 @@ static int domain_event_cb(virConnectPtr conn G_GNUC_UNUSED, void *opaque) { gchar uuid[VIR_UUID_STRING_BUFLEN]; + GHashTable *doms; GVirConnection *gconn = opaque; GVirDomain *gdom; GVirConnectionPrivate *priv = gconn->priv; @@ -269,14 +270,18 @@ static int domain_event_cb(virConnectPtr conn G_GNUC_UNUSED, g_debug("%s: %s event:%d, detail:%d", G_STRFUNC, uuid, event, detail); g_mutex_lock(priv->lock); - gdom = g_hash_table_lookup(priv->domains, uuid); + doms = g_hash_table_ref(priv->domains); + gdom = g_hash_table_lookup(doms, uuid); + if (gdom != NULL) + g_object_ref(G_OBJECT(gdom)); g_mutex_unlock(priv->lock); if (gdom == NULL) { gdom = GVIR_DOMAIN(g_object_new(GVIR_TYPE_DOMAIN, "handle", dom, NULL)); g_mutex_lock(priv->lock); - g_hash_table_insert(priv->domains, (gpointer)gvir_domain_get_uuid(gdom), gdom); + g_hash_table_insert(doms, (gpointer)gvir_domain_get_uuid(gdom), + g_object_ref(G_OBJECT(gdom))); g_mutex_unlock(priv->lock); } @@ -293,11 +298,10 @@ static int domain_event_cb(virConnectPtr conn G_GNUC_UNUSED, case VIR_DOMAIN_EVENT_UNDEFINED: if (detail == VIR_DOMAIN_EVENT_UNDEFINED_REMOVED) { g_mutex_lock(priv->lock); - g_hash_table_steal(priv->domains, uuid); + g_hash_table_remove(doms, uuid); g_mutex_unlock(priv->lock); g_signal_emit(gconn, signals[VIR_DOMAIN_REMOVED], 0, gdom); - g_object_unref(gdom); } else g_warn_if_reached(); break; @@ -365,11 +369,10 @@ static int domain_event_cb(virConnectPtr conn G_GNUC_UNUSED, if (virDomainIsPersistent(dom) != 1) { g_mutex_lock(priv->lock); - g_hash_table_steal(priv->domains, uuid); + g_hash_table_remove(doms, uuid); g_mutex_unlock(priv->lock); g_signal_emit(gconn, signals[VIR_DOMAIN_REMOVED], 0, gdom); - g_object_unref(gdom); } break; @@ -377,6 +380,8 @@ static int domain_event_cb(virConnectPtr conn G_GNUC_UNUSED, g_warn_if_reached(); } + g_object_unref(G_OBJECT(gdom)); + g_hash_table_unref(doms); return 0; }