File glib2-allocate-SignalSubscriber-structs-individ... of Package glib2.36447

Overview Repositories Revisions Requests Users Attributes Meta

File glib2-allocate-SignalSubscriber-structs-individually.patch of Package glib2.36447

From 9b1c8d7dd52c69962e58978f58a82bd703280735 Mon Sep 17 00:00:00 2001
From: Philip Withnall <withnall@endlessm.com>
Date: Fri, 17 Jan 2020 16:11:06 +0000
Subject: [PATCH] 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 -urp glib-2.48.2.orig/gio/gdbusconnection.c glib-2.48.2/gio/gdbusconnection.c
--- glib-2.48.2.orig/gio/gdbusconnection.c	2024-05-15 16:36:58.516376538 -0500
+++ glib-2.48.2/gio/gdbusconnection.c	2024-05-15 16:41:24.304855315 -0500
@@ -3208,18 +3208,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)
 {
@@ -3230,10 +3221,37 @@ signal_data_free (SignalData *signal_dat
   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,
@@ -3419,7 +3437,7 @@ g_dbus_connection_signal_subscribe (GDBu
 {
   gchar *rule;
   SignalData *signal_data;
-  SignalSubscriber subscriber;
+  SignalSubscriber *subscriber;
   GPtrArray *signal_data_array;
   const gchar *sender_unique_name;
 
@@ -3460,17 +3478,19 @@ g_dbus_connection_signal_subscribe (GDBu
   else
     sender_unique_name = "";
 
-  subscriber.callback = callback;
-  subscriber.user_data = user_data;
-  subscriber.user_data_free_func = user_data_free_func;
-  subscriber.id = _global_subscriber_id++; /* 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;
     }
@@ -3484,8 +3504,8 @@ g_dbus_connection_signal_subscribe (GDBu
   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,
@@ -3515,12 +3535,12 @@ g_dbus_connection_signal_subscribe (GDBu
 
  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;
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
@@ -3530,7 +3550,7 @@ g_dbus_connection_signal_subscribe (GDBu
 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;
@@ -3546,16 +3566,19 @@ unsubscribe_id_internal (GDBusConnection
 
   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)
         {
@@ -3613,13 +3636,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,
@@ -3633,15 +3656,15 @@ g_dbus_connection_signal_unsubscribe (GD
   /* 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);
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
@@ -3831,12 +3854,10 @@ schedule_callbacks (GDBusConnection *con
 
       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;
@@ -3909,7 +3930,7 @@ purge_all_signal_subscriptions (GDBusCon
   GHashTableIter iter;
   gpointer key;
   GArray *ids;
-  GArray *subscribers;
+  GPtrArray *subscribers;
   guint n;
 
   ids = g_array_new (FALSE, FALSE, sizeof (guint));
@@ -3920,7 +3941,7 @@ purge_all_signal_subscriptions (GDBusCon
       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);
@@ -3933,15 +3954,15 @@ purge_all_signal_subscriptions (GDBusCon
   /* 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);
 }
 
 /* ---------------------------------------------------------------------------------------------------- */

Places

File glib2-allocate-SignalSubscriber-structs-individually.patch of Package glib2.36447

Places