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

[Christophe Fergeau <cfergeau@xxxxxxxxxx>]

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;
 }
 
-- 
1.7.7.3

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list