util: avoid crash due to race in glib event loop code

There is a fairly long standing race condition bug in glib which can hit
if you call g_source_destroy or g_source_unref from a non-main thread:

  https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358

Unfortunately it is really common for libvirt to call g_source_destroy
from a non-main thread. This glib bug is the cause of non-determinstic
crashes in eventtest, and probably in libvirtd too.

To work around the problem we need to ensure that we never release
the last reference on a GSource from a non-main thread. The previous
patch replaced our use of g_source_destroy with a pair of
g_source_remove and g_source_unref. We can now delay the g_source_unref
call by using a idle callback to invoke it from the main thread which
avoids the race condition.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c
index 6e334b33982f03a17ba6be60e4e2c574a16597fe..7e61b096ed9da747847e20f264486da52b5a51de 100644 (file)
--- a/src/util/vireventglib.c
+++ b/src/util/vireventglib.c
@@ -185,6 +185,24 @@ virEventGLibHandleFind(int watch)
     return NULL;
 }
 
+/*
+ * If the last refernce to a GSource is released in a non-main
+ * thread we're exposed to a race condition that causes a
+ * crash:
+ * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
+ * Thus we're using an idle func to release our ref
+ */
+static gboolean
+virEventGLibSourceUnrefIdle(gpointer data)
+{
+    GSource *src = data;
+
+    g_source_unref(src);
+
+    return FALSE;
+}
+
+
 static void
 virEventGLibHandleUpdate(int watch,
                          int events)
@@ -213,7 +231,7 @@ virEventGLibHandleUpdate(int watch,
         if (data->source != NULL) {
             VIR_DEBUG("Removed old handle source=%p", data->source);
             g_source_destroy(data->source);
-            g_source_unref(data->source);
+            g_idle_add(virEventGLibSourceUnrefIdle, data->source);
         }
 
         data->source = virEventGLibAddSocketWatch(
@@ -227,7 +245,7 @@ virEventGLibHandleUpdate(int watch,
 
         VIR_DEBUG("Removed old handle source=%p", data->source);
         g_source_destroy(data->source);
-        g_source_unref(data->source);
+        g_idle_add(virEventGLibSourceUnrefIdle, data->source);
         data->source = NULL;
         data->events = 0;
     }
@@ -276,7 +294,7 @@ virEventGLibHandleRemove(int watch)
 
     if (data->source != NULL) {
         g_source_destroy(data->source);
-        g_source_unref(data->source);
+        g_idle_add(virEventGLibSourceUnrefIdle, data->source);
         data->source = NULL;
         data->events = 0;
     }
@@ -409,7 +427,7 @@ virEventGLibTimeoutUpdate(int timer,
     if (interval >= 0) {
         if (data->source != NULL) {
             g_source_destroy(data->source);
-            g_source_unref(data->source);
+            g_idle_add(virEventGLibSourceUnrefIdle, data->source);
         }
 
         data->interval = interval;
@@ -419,7 +437,7 @@ virEventGLibTimeoutUpdate(int timer,
             goto cleanup;
 
         g_source_destroy(data->source);
-        g_source_unref(data->source);
+        g_idle_add(virEventGLibSourceUnrefIdle, data->source);
         data->source = NULL;
     }
 
@@ -468,7 +486,7 @@ virEventGLibTimeoutRemove(int timer)
 
     if (data->source != NULL) {
         g_source_destroy(data->source);
-        g_source_unref(data->source);
+        g_idle_add(virEventGLibSourceUnrefIdle, data->source);
         data->source = NULL;
     }