[glib: 2/8] gdbusconnection: Allocate SignalSubscriber structs individually

[Philip Withnall <pwithnall src gnome org>]

commit 9b1c8d7dd52c69962e58978f58a82bd703280735
Author: Philip Withnall <withnall endlessm com>
Date:   Fri Jan 17 16:11:06 2020 +0000

    gdbusconnection: Allocate SignalSubscriber structs individually
    
    The `SignalSubscriber` structs contain the callback and `user_data` of each
    subscriber to a signal, along with the `guint id` token held by that
    subscriber to identify their subscription. There are one or more
    `SignalSubscriber` structs for a given signal match rule, which is
    represented as a `SignalData` struct.
    
    Previously, the `SignalSubscriber` structs were stored in a `GArray` in
    the `SignalData` struct, to reduce the number of allocations needed
    when subscribing to a signal.
    
    However, this means that a `SignalSubscriber` struct cannot have a
    lifetime which exceeds the `SignalData` which contains it. In order to
    fix the race in #978, one thread needs to be able to unsubscribe from a
    signal (destroying the `SignalData` struct) while zero or more other
    threads are in the process of calling the callbacks from a previous
    emission of that signal (using the callback and `user_data` from zero or
    more `SignalSubscriber` structs). Multiple threads could be calling
    callbacks because callbacks are invoked in the `GMainContext` which
    originally made a subscription, and GDBus supports subscribing to a
    signal from multiple threads. In that case, the callbacks are dispatched
    to multiple threads.
    
    In order to allow the `SignalSubscriber` structs to outlive the
    `SignalData` which contained their old match rule, store them in a
    `GPtrArray` in the `SignalData` struct, and refcount them individually.
    
    This commit in itself should make no functional changes to how GDBus
    works, but will allow following commits to do so.
    
    Signed-off-by: Philip Withnall <withnall endlessm com>
    
    Helps: #978

 gio/gdbusconnection.c | 105 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 63 insertions(+), 42 deletions(-)
---
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index dbfc53953..f2cba14cd 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -3248,18 +3248,9 @@ typedef struct
   gchar *object_path;
   gchar *arg0;
   GDBusSignalFlags flags;
-  GArray *subscribers;
+  GPtrArray *subscribers;  /* (owned) (element-type SignalSubscriber) */
 } SignalData;
 
-typedef struct
-{
-  GDBusSignalCallback callback;
-  gpointer user_data;
-  GDestroyNotify user_data_free_func;
-  guint id;
-  GMainContext *context;
-} SignalSubscriber;
-
 static void
 signal_data_free (SignalData *signal_data)
 {
@@ -3270,10 +3261,37 @@ signal_data_free (SignalData *signal_data)
   g_free (signal_data->member);
   g_free (signal_data->object_path);
   g_free (signal_data->arg0);
-  g_array_free (signal_data->subscribers, TRUE);
+  g_ptr_array_unref (signal_data->subscribers);
   g_free (signal_data);
 }
 
+typedef struct
+{
+  gatomicrefcount ref_count;
+  GDBusSignalCallback callback;
+  gpointer user_data;
+  GDestroyNotify user_data_free_func;
+  guint id;
+  GMainContext *context;
+} SignalSubscriber;
+
+static SignalSubscriber *
+signal_subscriber_ref (SignalSubscriber *subscriber)
+{
+  g_atomic_ref_count_inc (&subscriber->ref_count);
+  return subscriber;
+}
+
+static void
+signal_subscriber_unref (SignalSubscriber *subscriber)
+{
+  if (g_atomic_ref_count_dec (&subscriber->ref_count))
+    {
+      g_main_context_unref (subscriber->context);
+      g_free (subscriber);
+    }
+}
+
 static gchar *
 args_to_rule (const gchar      *sender,
               const gchar      *interface_name,
@@ -3464,7 +3482,7 @@ g_dbus_connection_signal_subscribe (GDBusConnection     *connection,
 {
   gchar *rule;
   SignalData *signal_data;
-  SignalSubscriber subscriber;
+  SignalSubscriber *subscriber;
   GPtrArray *signal_data_array;
   const gchar *sender_unique_name;
 
@@ -3505,17 +3523,19 @@ g_dbus_connection_signal_subscribe (GDBusConnection     *connection,
   else
     sender_unique_name = "";
 
-  subscriber.callback = callback;
-  subscriber.user_data = user_data;
-  subscriber.user_data_free_func = user_data_free_func;
-  subscriber.id = (guint) g_atomic_int_add (&_global_subscriber_id, 1); /* TODO: overflow etc. */
-  subscriber.context = g_main_context_ref_thread_default ();
+  subscriber = g_new0 (SignalSubscriber, 1);
+  subscriber->ref_count = 1;
+  subscriber->callback = callback;
+  subscriber->user_data = user_data;
+  subscriber->user_data_free_func = user_data_free_func;
+  subscriber->id = (guint) g_atomic_int_add (&_global_subscriber_id, 1); /* TODO: overflow etc. */
+  subscriber->context = g_main_context_ref_thread_default ();
 
   /* see if we've already have this rule */
   signal_data = g_hash_table_lookup (connection->map_rule_to_signal_data, rule);
   if (signal_data != NULL)
     {
-      g_array_append_val (signal_data->subscribers, subscriber);
+      g_ptr_array_add (signal_data->subscribers, subscriber);
       g_free (rule);
       goto out;
     }
@@ -3529,8 +3549,8 @@ g_dbus_connection_signal_subscribe (GDBusConnection     *connection,
   signal_data->object_path           = g_strdup (object_path);
   signal_data->arg0                  = g_strdup (arg0);
   signal_data->flags                 = flags;
-  signal_data->subscribers           = g_array_new (FALSE, FALSE, sizeof (SignalSubscriber));
-  g_array_append_val (signal_data->subscribers, subscriber);
+  signal_data->subscribers           = g_ptr_array_new_with_free_func ((GDestroyNotify) 
signal_subscriber_unref);
+  g_ptr_array_add (signal_data->subscribers, subscriber);
 
   g_hash_table_insert (connection->map_rule_to_signal_data,
                        signal_data->rule,
@@ -3560,12 +3580,12 @@ g_dbus_connection_signal_subscribe (GDBusConnection     *connection,
 
  out:
   g_hash_table_insert (connection->map_id_to_signal_data,
-                       GUINT_TO_POINTER (subscriber.id),
+                       GUINT_TO_POINTER (subscriber->id),
                        signal_data);
 
   CONNECTION_UNLOCK (connection);
 
-  return subscriber.id;
+  return subscriber->id;
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
@@ -3575,7 +3595,7 @@ g_dbus_connection_signal_subscribe (GDBusConnection     *connection,
 static void
 unsubscribe_id_internal (GDBusConnection *connection,
                          guint            subscription_id,
-                         GArray          *out_removed_subscribers)
+                         GPtrArray       *out_removed_subscribers)
 {
   SignalData *signal_data;
   GPtrArray *signal_data_array;
@@ -3591,16 +3611,19 @@ unsubscribe_id_internal (GDBusConnection *connection,
 
   for (n = 0; n < signal_data->subscribers->len; n++)
     {
-      SignalSubscriber *subscriber;
+      SignalSubscriber *subscriber = signal_data->subscribers->pdata[n];
 
-      subscriber = &(g_array_index (signal_data->subscribers, SignalSubscriber, n));
       if (subscriber->id != subscription_id)
         continue;
 
+      /* It’s OK to rearrange the array order using the ‘fast’ #GPtrArray
+       * removal functions, since we’re going to exit the loop below anyway — we
+       * never move on to the next element. Secondly, subscription IDs are
+       * guaranteed to be unique. */
       g_warn_if_fail (g_hash_table_remove (connection->map_id_to_signal_data,
                                            GUINT_TO_POINTER (subscription_id)));
-      g_array_append_val (out_removed_subscribers, *subscriber);
-      g_array_remove_index (signal_data->subscribers, n);
+      g_ptr_array_add (out_removed_subscribers, signal_subscriber_ref (subscriber));
+      g_ptr_array_remove_index_fast (signal_data->subscribers, n);
 
       if (signal_data->subscribers->len == 0)
         {
@@ -3658,13 +3681,13 @@ void
 g_dbus_connection_signal_unsubscribe (GDBusConnection *connection,
                                       guint            subscription_id)
 {
-  GArray *subscribers;
+  GPtrArray *subscribers;
   guint n;
 
   g_return_if_fail (G_IS_DBUS_CONNECTION (connection));
   g_return_if_fail (check_initialized (connection));
 
-  subscribers = g_array_new (FALSE, FALSE, sizeof (SignalSubscriber));
+  subscribers = g_ptr_array_new_with_free_func ((GDestroyNotify) signal_subscriber_unref);
 
   CONNECTION_LOCK (connection);
   unsubscribe_id_internal (connection,
@@ -3678,15 +3701,15 @@ g_dbus_connection_signal_unsubscribe (GDBusConnection *connection,
   /* call GDestroyNotify without lock held */
   for (n = 0; n < subscribers->len; n++)
     {
-      SignalSubscriber *subscriber;
-      subscriber = &(g_array_index (subscribers, SignalSubscriber, n));
+      SignalSubscriber *subscriber = subscribers->pdata[n];
       call_destroy_notify (subscriber->context,
                            subscriber->user_data_free_func,
                            subscriber->user_data);
-      g_main_context_unref (subscriber->context);
+      subscriber->user_data_free_func = NULL;
+      subscriber->user_data = NULL;
     }
 
-  g_array_free (subscribers, TRUE);
+  g_ptr_array_unref (subscribers);
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
@@ -3876,12 +3899,10 @@ schedule_callbacks (GDBusConnection *connection,
 
       for (m = 0; m < signal_data->subscribers->len; m++)
         {
-          SignalSubscriber *subscriber;
+          SignalSubscriber *subscriber = signal_data->subscribers->pdata[m];
           GSource *idle_source;
           SignalInstance *signal_instance;
 
-          subscriber = &(g_array_index (signal_data->subscribers, SignalSubscriber, m));
-
           signal_instance = g_new0 (SignalInstance, 1);
           signal_instance->subscription_id = subscriber->id;
           signal_instance->callback = subscriber->callback;
@@ -3954,7 +3975,7 @@ purge_all_signal_subscriptions (GDBusConnection *connection)
   GHashTableIter iter;
   gpointer key;
   GArray *ids;
-  GArray *subscribers;
+  GPtrArray *subscribers;
   guint n;
 
   ids = g_array_new (FALSE, FALSE, sizeof (guint));
@@ -3965,7 +3986,7 @@ purge_all_signal_subscriptions (GDBusConnection *connection)
       g_array_append_val (ids, subscription_id);
     }
 
-  subscribers = g_array_new (FALSE, FALSE, sizeof (SignalSubscriber));
+  subscribers = g_ptr_array_new_with_free_func ((GDestroyNotify) signal_subscriber_unref);
   for (n = 0; n < ids->len; n++)
     {
       guint subscription_id = g_array_index (ids, guint, n);
@@ -3978,15 +3999,15 @@ purge_all_signal_subscriptions (GDBusConnection *connection)
   /* call GDestroyNotify without lock held */
   for (n = 0; n < subscribers->len; n++)
     {
-      SignalSubscriber *subscriber;
-      subscriber = &(g_array_index (subscribers, SignalSubscriber, n));
+      SignalSubscriber *subscriber = subscribers->pdata[n];
       call_destroy_notify (subscriber->context,
                            subscriber->user_data_free_func,
                            subscriber->user_data);
-      g_main_context_unref (subscriber->context);
+      subscriber->user_data_free_func = NULL;
+      subscriber->user_data = NULL;
     }
 
-  g_array_free (subscribers, TRUE);
+  g_ptr_array_unref (subscribers);
 }
 
 /* ---------------------------------------------------------------------------------------------------- */