[glib: 1/2] gdbusobjectmanagerclient: Fix race in signal emission

[Philip Withnall <pwithnall src gnome org>]

commit 5853d5c8e45d05b38c3360d949142a9b102a0eef
Author: Philip Withnall <withnall endlessm com>
Date:   Mon Jan 20 18:53:50 2020 +0000

    gdbusobjectmanagerclient: Fix race in signal emission
    
    Following on from #978, it seems that #1232 is another instance of the
    same problem: signals emitted across threads can’t guarantee their user
    data is kept alive between committing to emitting the signal and
    actually invoking the callback in the relevant thread.
    
    Fix that by using weak refs to the `GDBusObjectManagerClient` as the
    user data for its signals, rather than no refs. Strong refs would create
    an unbreakable reference count cycle.
    
    Signed-off-by: Philip Withnall <withnall endlessm com>
    
    Fixes: #1232

 gio/gdbusobjectmanagerclient.c | 102 ++++++++++++++++++++++++++++++++---------
 1 file changed, 80 insertions(+), 22 deletions(-)
---
diff --git a/gio/gdbusobjectmanagerclient.c b/gio/gdbusobjectmanagerclient.c
index 38e6f539f..a9b94f69d 100644
--- a/gio/gdbusobjectmanagerclient.c
+++ b/gio/gdbusobjectmanagerclient.c
@@ -141,6 +141,9 @@ struct _GDBusObjectManagerClientPrivate
   GDBusProxyTypeFunc get_proxy_type_func;
   gpointer get_proxy_type_user_data;
   GDestroyNotify get_proxy_type_destroy_notify;
+
+  gulong name_owner_signal_id;
+  gulong signal_signal_id;
 };
 
 enum
@@ -197,13 +200,18 @@ g_dbus_object_manager_client_finalize (GObject *object)
 
   g_hash_table_unref (manager->priv->map_object_path_to_object_proxy);
 
-  if (manager->priv->control_proxy != NULL)
-    {
-      g_signal_handlers_disconnect_by_func (manager->priv->control_proxy,
-                                            on_control_proxy_g_signal,
-                                            manager);
-      g_object_unref (manager->priv->control_proxy);
-    }
+  if (manager->priv->control_proxy != NULL && manager->priv->signal_signal_id != 0)
+    g_signal_handler_disconnect (manager->priv->control_proxy,
+                                 manager->priv->signal_signal_id);
+  manager->priv->signal_signal_id = 0;
+
+  if (manager->priv->control_proxy != NULL && manager->priv->name_owner_signal_id != 0)
+    g_signal_handler_disconnect (manager->priv->control_proxy,
+                                 manager->priv->name_owner_signal_id);
+  manager->priv->name_owner_signal_id = 0;
+
+  g_clear_object (&manager->priv->control_proxy);
+
   if (manager->priv->connection != NULL)
     g_object_unref (manager->priv->connection);
   g_free (manager->priv->object_path);
@@ -1241,16 +1249,20 @@ on_notify_g_name_owner (GObject    *object,
                         GParamSpec *pspec,
                         gpointer    user_data)
 {
-  GDBusObjectManagerClient *manager = G_DBUS_OBJECT_MANAGER_CLIENT (user_data);
+  GWeakRef *manager_weak = user_data;
+  GDBusObjectManagerClient *manager = NULL;
   gchar *old_name_owner;
   gchar *new_name_owner;
 
+  manager = G_DBUS_OBJECT_MANAGER_CLIENT (g_weak_ref_get (manager_weak));
+  if (manager == NULL)
+    return;
+
   g_mutex_lock (&manager->priv->lock);
   old_name_owner = manager->priv->name_owner;
   new_name_owner = g_dbus_proxy_get_name_owner (manager->priv->control_proxy);
   manager->priv->name_owner = NULL;
 
-  g_object_ref (manager);
   if (g_strcmp0 (old_name_owner, new_name_owner) != 0)
     {
       GList *l;
@@ -1330,6 +1342,21 @@ on_notify_g_name_owner (GObject    *object,
   g_object_unref (manager);
 }
 
+static GWeakRef *
+weak_ref_new (GObject *object)
+{
+  GWeakRef *weak_ref = g_new0 (GWeakRef, 1);
+  g_weak_ref_init (weak_ref, object);
+  return g_steal_pointer (&weak_ref);
+}
+
+static void
+weak_ref_free (GWeakRef *weak_ref)
+{
+  g_weak_ref_clear (weak_ref);
+  g_free (weak_ref);
+}
+
 static gboolean
 initable_init (GInitable     *initable,
                GCancellable  *cancellable,
@@ -1365,15 +1392,30 @@ initable_init (GInitable     *initable,
   if (manager->priv->control_proxy == NULL)
     goto out;
 
-  g_signal_connect (G_OBJECT (manager->priv->control_proxy),
-                    "notify::g-name-owner",
-                    G_CALLBACK (on_notify_g_name_owner),
-                    manager);
-
-  g_signal_connect (manager->priv->control_proxy,
-                    "g-signal",
-                    G_CALLBACK (on_control_proxy_g_signal),
-                    manager);
+  /* Use weak refs here. The @control_proxy will emit its signals in the current
+   * #GMainContext (since we constructed it just above). However, the user may
+   * drop the last external reference to this #GDBusObjectManagerClient in
+   * another thread between a signal being emitted and scheduled in an idle
+   * callback in this #GMainContext, and that idle callback being invoked. We
+   * can’t use a strong reference here, as there’s no
+   * g_dbus_object_manager_client_disconnect() (or similar) method to tell us
+   * when the last external reference to this object has been dropped, so we
+   * can’t break a strong reference count cycle. So use weak refs. */
+  manager->priv->name_owner_signal_id =
+      g_signal_connect_data (G_OBJECT (manager->priv->control_proxy),
+                            "notify::g-name-owner",
+                            G_CALLBACK (on_notify_g_name_owner),
+                            weak_ref_new (G_OBJECT (manager)),
+                            (GClosureNotify) weak_ref_free,
+                            0  /* flags */);
+
+  manager->priv->signal_signal_id =
+      g_signal_connect_data (manager->priv->control_proxy,
+                            "g-signal",
+                            G_CALLBACK (on_control_proxy_g_signal),
+                            weak_ref_new (G_OBJECT (manager)),
+                            (GClosureNotify) weak_ref_free,
+                            0  /* flags */);
 
   manager->priv->name_owner = g_dbus_proxy_get_name_owner (manager->priv->control_proxy);
   if (manager->priv->name_owner == NULL && manager->priv->name != NULL)
@@ -1397,11 +1439,20 @@ initable_init (GInitable     *initable,
       if (value == NULL)
         {
           maybe_unsubscribe_signals (manager);
-          g_warn_if_fail (g_signal_handlers_disconnect_by_func (manager->priv->control_proxy,
-                                                                on_control_proxy_g_signal,
-                                                                manager) == 1);
+
+          g_warn_if_fail (manager->priv->signal_signal_id != 0);
+          g_signal_handler_disconnect (manager->priv->control_proxy,
+                                       manager->priv->signal_signal_id);
+          manager->priv->signal_signal_id = 0;
+
+          g_warn_if_fail (manager->priv->name_owner_signal_id != 0);
+          g_signal_handler_disconnect (manager->priv->control_proxy,
+                                       manager->priv->name_owner_signal_id);
+          manager->priv->name_owner_signal_id = 0;
+
           g_object_unref (manager->priv->control_proxy);
           manager->priv->control_proxy = NULL;
+
           goto out;
         }
 
@@ -1668,9 +1719,14 @@ on_control_proxy_g_signal (GDBusProxy   *proxy,
                            GVariant     *parameters,
                            gpointer      user_data)
 {
-  GDBusObjectManagerClient *manager = G_DBUS_OBJECT_MANAGER_CLIENT (user_data);
+  GWeakRef *manager_weak = user_data;
+  GDBusObjectManagerClient *manager = NULL;
   const gchar *object_path;
 
+  manager = G_DBUS_OBJECT_MANAGER_CLIENT (g_weak_ref_get (manager_weak));
+  if (manager == NULL)
+    return;
+
   //g_debug ("yay, g_signal %s: %s\n", signal_name, g_variant_print (parameters, TRUE));
 
   if (g_strcmp0 (signal_name, "InterfacesAdded") == 0)
@@ -1693,6 +1749,8 @@ on_control_proxy_g_signal (GDBusProxy   *proxy,
       remove_interfaces (manager, object_path, ifaces);
       g_free (ifaces);
     }
+
+  g_object_unref (manager);
 }
 
 /* ---------------------------------------------------------------------------------------------------- */