File glib2-allocate-SignalSubscriber-structs-individually.patch of Package glib2.34039
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);
}
/* ---------------------------------------------------------------------------------------------------- */
