From 56aa0a2d05d92b39f4bd4150cca4441487af24f2 Mon Sep 17 00:00:00 2001
From: Simon McVittie <smcv@collabora.com>
Date: Tue, 23 Apr 2024 20:42:17 +0100
Subject: [PATCH 12/17] gdbus: Track name owners for signal subscriptions
We will use this in a subsequent commit to prevent signals from an
impostor from being delivered to a subscriber.
To avoid message reordering leading to misleading situations, this does
not use the existing mechanism for watching bus name ownership, which
delivers the ownership changes to other main-contexts. Instead, it all
happens on the single thread used by the GDBusWorker, so the order in
which messages are received is the order in which they are processed.
Signed-off-by: Simon McVittie <smcv@collabora.com>
---
gio/gdbusconnection.c | 350 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 343 insertions(+), 7 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 805ef1870c..0a5f9b691c 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -321,6 +321,31 @@ signal_subscriber_unref (SignalSubscriber *subscriber)
}
typedef struct
+{
+ /*
+ * 1 reference while waiting for GetNameOwner() to finish
+ * 1 reference for each SignalData that points to this one as its
+ * shared_name_watcher
+ */
+ grefcount ref_count;
+
+ gchar *owner;
+ guint32 get_name_owner_serial;
+} WatchedName;
+
+static WatchedName *
+watched_name_new (void)
+{
+ WatchedName *watched_name = g_new0 (WatchedName, 1);
+
+ g_ref_count_init (&watched_name->ref_count);
+ watched_name->owner = NULL;
+ return g_steal_pointer (&watched_name);
+}
+
+typedef struct SignalData SignalData;
+
+struct SignalData
{
gchar *rule;
gchar *sender;
@@ -330,13 +355,36 @@ typedef struct
gchar *arg0;
GDBusSignalFlags flags;
GPtrArray *subscribers; /* (owned) (element-type SignalSubscriber) */
- gboolean sender_is_its_own_owner;
-} SignalData;
+
+ /*
+ * If the sender is a well-known name, this is an unowned SignalData
+ * representing the NameOwnerChanged signal that tracks its owner.
+ * NULL if sender is NULL.
+ * NULL if sender is its own owner (a unique name or DBUS_SERVICE_DBUS).
+ *
+ * Invariants: if not NULL, then
+ * shared_name_watcher->sender == DBUS_SERVICE_DBUS
+ * shared_name_watcher->interface_name == DBUS_INTERFACE_DBUS
+ * shared_name_watcher->member == "NameOwnerChanged"
+ * shared_name_watcher->object_path == DBUS_PATH_DBUS
+ * shared_name_watcher->arg0 == sender
+ * shared_name_watcher->flags == NONE
+ * shared_name_watcher->watched_name == NULL
+ */
+ SignalData *shared_name_watcher;
+
+ /*
+ * Non-NULL if this SignalData is another SignalData's shared_name_watcher.
+ * One reference for each SignalData that has this one as its
+ * shared_name_watcher.
+ * Otherwise NULL.
+ */
+ WatchedName *watched_name;
+};
static SignalData *
signal_data_new_take (gchar *rule,
gchar *sender,
- gboolean sender_is_its_own_owner,
gchar *interface_name,
gchar *member,
gchar *object_path,
@@ -347,7 +395,6 @@ signal_data_new_take (gchar *rule,
signal_data->rule = rule;
signal_data->sender = sender;
- signal_data->sender_is_its_own_owner = sender_is_its_own_owner;
signal_data->interface_name = interface_name;
signal_data->member = member;
signal_data->object_path = object_path;
@@ -360,6 +407,17 @@ signal_data_new_take (gchar *rule,
static void
signal_data_free (SignalData *signal_data)
{
+ /* The SignalData should not be freed while it still has subscribers */
+ g_assert (signal_data->subscribers->len == 0);
+
+ /* The SignalData should not be freed while it is watching for
+ * NameOwnerChanged on behalf of another SignalData */
+ g_assert (signal_data->watched_name == NULL);
+
+ /* The SignalData should be detached from its name watcher, if any,
+ * before it is freed */
+ g_assert (signal_data->shared_name_watcher == NULL);
+
g_free (signal_data->rule);
g_free (signal_data->sender);
g_free (signal_data->interface_name);
@@ -367,6 +425,7 @@ signal_data_free (SignalData *signal_data)
g_free (signal_data->object_path);
g_free (signal_data->arg0);
g_ptr_array_unref (signal_data->subscribers);
+
g_free (signal_data);
}
@@ -498,6 +557,7 @@ struct _GDBusConnection
/* Map used for managing method replies, protected by @lock */
GHashTable *map_method_serial_to_task; /* guint32 -> owned GTask* */
+ GHashTable *map_method_serial_to_name_watcher; /* guint32 -> unowned SignalData* */
/* Maps used for managing signal subscription, protected by @lock */
GHashTable *map_rule_to_signal_data; /* match rule (gchar*) -> SignalData */
@@ -746,6 +806,7 @@ g_dbus_connection_finalize (GObject *object)
g_error_free (connection->initialization_error);
g_hash_table_unref (connection->map_method_serial_to_task);
+ g_hash_table_unref (connection->map_method_serial_to_name_watcher);
g_hash_table_unref (connection->map_rule_to_signal_data);
g_hash_table_unref (connection->map_id_to_signal_data);
@@ -1150,6 +1211,7 @@ g_dbus_connection_init (GDBusConnection *connection)
g_mutex_init (&connection->init_lock);
connection->map_method_serial_to_task = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref);
+ connection->map_method_serial_to_name_watcher = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, NULL);
connection->map_rule_to_signal_data = g_hash_table_new (g_str_hash,
g_str_equal);
@@ -2277,6 +2339,191 @@ g_dbus_connection_send_message_with_reply_sync (GDBusConnection *connecti
/* ---------------------------------------------------------------------------------------------------- */
+/*
+ * Called in any thread.
+ * Must hold the connection lock when calling this, unless
+ * connection->finalizing is TRUE.
+ */
+static void
+name_watcher_unref_watched_name (GDBusConnection *connection,
+ SignalData *name_watcher)
+{
+ WatchedName *watched_name = name_watcher->watched_name;
+
+ g_assert (watched_name != NULL);
+
+ if (!g_ref_count_dec (&watched_name->ref_count))
+ return;
+
+ /* Removing watched_name from the name_watcher may result in
+ * name_watcher being freed, so we must make sure name_watcher is no
+ * longer in map_method_serial_to_name_watcher.
+ *
+ * If we stop watching the name while our GetNameOwner call was still
+ * in-flight, then when the reply eventually arrives, we will not find
+ * its serial number in the map and harmlessly ignore it as a result. */
+ if (watched_name->get_name_owner_serial != 0)
+ g_hash_table_remove (connection->map_method_serial_to_name_watcher,
+ GUINT_TO_POINTER (watched_name->get_name_owner_serial));
+
+ name_watcher->watched_name = NULL;
+ g_free (watched_name->owner);
+ g_free (watched_name);
+}
+
+/* called in GDBusWorker thread with lock held */
+static void
+name_watcher_set_name_owner_unlocked (SignalData *name_watcher,
+ const char *new_owner)
+{
+ if (new_owner != NULL && new_owner[0] == '\0')
+ new_owner = NULL;
+
+ g_assert (name_watcher->watched_name != NULL);
+ g_set_str (&name_watcher->watched_name->owner, new_owner);
+}
+
+/* called in GDBusWorker thread with lock held */
+static void
+name_watcher_deliver_name_owner_changed_unlocked (SignalData *name_watcher,
+ GDBusMessage *message)
+{
+ GVariant *body;
+
+ body = g_dbus_message_get_body (message);
+
+ if (G_LIKELY (body != NULL && g_variant_is_of_type (body, G_VARIANT_TYPE ("(sss)"))))
+ {
+ const char *name;
+ const char *new_owner;
+
+ g_variant_get (body, "(&s&s&s)", &name, NULL, &new_owner);
+
+ /* Our caller already checked this */
+ g_assert (g_strcmp0 (name_watcher->arg0, name) == 0);
+
+ if (G_LIKELY (new_owner[0] == '\0' || g_dbus_is_unique_name (new_owner)))
+ name_watcher_set_name_owner_unlocked (name_watcher, new_owner);
+ else
+ g_warning ("Received NameOwnerChanged signal with invalid owner \"%s\" for \"%s\"",
+ new_owner, name);
+ }
+ else
+ {
+ g_warning ("Received NameOwnerChanged signal with unexpected "
+ "signature %s",
+ body == NULL ? "()" : g_variant_get_type_string (body));
+
+ }
+}
+
+/* called in GDBusWorker thread with lock held */
+static void
+name_watcher_deliver_get_name_owner_reply_unlocked (SignalData *name_watcher,
+ GDBusConnection *connection,
+ GDBusMessage *message)
+{
+ GDBusMessageType type;
+ GVariant *body;
+ WatchedName *watched_name;
+
+ watched_name = name_watcher->watched_name;
+ g_assert (watched_name != NULL);
+ g_assert (watched_name->get_name_owner_serial != 0);
+
+ type = g_dbus_message_get_message_type (message);
+ body = g_dbus_message_get_body (message);
+
+ if (type == G_DBUS_MESSAGE_TYPE_ERROR)
+ {
+ if (g_strcmp0 (g_dbus_message_get_error_name (message),
+ "org.freedesktop.DBus.Error.NameHasNoOwner"))
+ name_watcher_set_name_owner_unlocked (name_watcher, NULL);
+ /* else it's something like NoReply or AccessDenied, which tells
+ * us nothing - leave the owner set to whatever we most recently
+ * learned from NameOwnerChanged, or NULL */
+ }
+ else if (type != G_DBUS_MESSAGE_TYPE_METHOD_RETURN)
+ {
+ g_warning ("Received GetNameOwner reply with unexpected type %d",
+ type);
+ }
+ else if (G_LIKELY (body != NULL && g_variant_is_of_type (body, G_VARIANT_TYPE ("(s)"))))
+ {
+ const char *new_owner;
+
+ g_variant_get (body, "(&s)", &new_owner);
+
+ if (G_LIKELY (g_dbus_is_unique_name (new_owner)))
+ name_watcher_set_name_owner_unlocked (name_watcher, new_owner);
+ else
+ g_warning ("Received GetNameOwner reply with invalid owner \"%s\" for \"%s\"",
+ new_owner, name_watcher->arg0);
+ }
+ else
+ {
+ g_warning ("Received GetNameOwner reply with unexpected signature %s",
+ body == NULL ? "()" : g_variant_get_type_string (body));
+ }
+
+ g_hash_table_remove (connection->map_method_serial_to_name_watcher,
+ GUINT_TO_POINTER (watched_name->get_name_owner_serial));
+ watched_name->get_name_owner_serial = 0;
+}
+
+/* Called in a user thread, lock is held */
+static void
+name_watcher_call_get_name_owner_unlocked (GDBusConnection *connection,
+ SignalData *name_watcher)
+{
+ GDBusMessage *message;
+ GError *local_error = NULL;
+ WatchedName *watched_name;
+
+ g_assert (g_strcmp0 (name_watcher->sender, DBUS_SERVICE_DBUS) == 0);
+ g_assert (g_strcmp0 (name_watcher->interface_name, DBUS_INTERFACE_DBUS) == 0);
+ g_assert (g_strcmp0 (name_watcher->member, "NameOwnerChanged") == 0);
+ g_assert (g_strcmp0 (name_watcher->object_path, DBUS_PATH_DBUS) == 0);
+ /* arg0 of the NameOwnerChanged message is the well-known name whose owner
+ * we are interested in */
+ g_assert (g_dbus_is_name (name_watcher->arg0));
+ g_assert (name_watcher->flags == G_DBUS_SIGNAL_FLAGS_NONE);
+
+ watched_name = name_watcher->watched_name;
+ g_assert (watched_name != NULL);
+ g_assert (watched_name->owner == NULL);
+ g_assert (watched_name->get_name_owner_serial == 0);
+ g_assert (name_watcher->shared_name_watcher == NULL);
+
+ message = g_dbus_message_new_method_call (DBUS_SERVICE_DBUS,
+ DBUS_PATH_DBUS,
+ DBUS_INTERFACE_DBUS,
+ "GetNameOwner");
+ g_dbus_message_set_body (message, g_variant_new ("(s)", name_watcher->arg0));
+
+ if (g_dbus_connection_send_message_unlocked (connection, message,
+ G_DBUS_SEND_MESSAGE_FLAGS_NONE,
+ &watched_name->get_name_owner_serial,
+ &local_error))
+ {
+ g_assert (watched_name->get_name_owner_serial != 0);
+ g_hash_table_insert (connection->map_method_serial_to_name_watcher,
+ GUINT_TO_POINTER (watched_name->get_name_owner_serial),
+ name_watcher);
+ }
+ else
+ {
+ g_critical ("Error while sending GetNameOwner() message: %s",
+ local_error->message);
+ g_clear_error (&local_error);
+ g_assert (watched_name->get_name_owner_serial == 0);
+ }
+
+ g_object_unref (message);
+}
+
+/* ---------------------------------------------------------------------------------------------------- */
+
typedef struct
{
guint id;
@@ -2400,6 +2647,7 @@ on_worker_message_received (GDBusWorker *worker,
{
guint32 reply_serial;
GTask *task;
+ SignalData *name_watcher;
reply_serial = g_dbus_message_get_reply_serial (message);
CONNECTION_LOCK (connection);
@@ -2415,6 +2663,19 @@ on_worker_message_received (GDBusWorker *worker,
{
//g_debug ("message reply/error for serial %d but no SendMessageData found for %p", reply_serial, connection);
}
+
+ name_watcher = g_hash_table_lookup (connection->map_method_serial_to_name_watcher,
+ GUINT_TO_POINTER (reply_serial));
+
+ if (name_watcher != NULL)
+ {
+ g_assert (name_watcher->watched_name != NULL);
+ g_assert (name_watcher->watched_name->get_name_owner_serial == reply_serial);
+ name_watcher_deliver_get_name_owner_reply_unlocked (name_watcher,
+ connection,
+ message);
+ }
+
CONNECTION_UNLOCK (connection);
}
else if (message_type == G_DBUS_MESSAGE_TYPE_SIGNAL)
@@ -3584,6 +3845,7 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
{
gchar *rule;
SignalData *signal_data;
+ SignalData *name_watcher = NULL;
SignalSubscriber *subscriber;
gboolean sender_is_its_own_owner;
const gchar *sender_unique_name;
@@ -3649,13 +3911,59 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
signal_data = signal_data_new_take (g_steal_pointer (&rule),
g_strdup (sender),
- sender_is_its_own_owner,
g_strdup (interface_name),
g_strdup (member),
g_strdup (object_path),
g_strdup (arg0),
flags);
g_ptr_array_add (signal_data->subscribers, subscriber);
+
+ /* If subscribing to a signal from a specific sender with a well-known
+ * name, we must first subscribe to NameOwnerChanged signals for that
+ * well-known name, so that we can match the current owner of the name
+ * against the sender of each signal. */
+ if (sender != NULL && !sender_is_its_own_owner)
+ {
+ gchar *name_owner_rule = NULL;
+
+ /* We already checked that sender != NULL implies MESSAGE_BUS_CONNECTION */
+ g_assert (connection->flags & G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION);
+
+ name_owner_rule = args_to_rule (DBUS_SERVICE_DBUS,
+ DBUS_INTERFACE_DBUS,
+ "NameOwnerChanged",
+ DBUS_PATH_DBUS,
+ sender,
+ G_DBUS_SIGNAL_FLAGS_NONE);
+ name_watcher = g_hash_table_lookup (connection->map_rule_to_signal_data, name_owner_rule);
+
+ if (name_watcher == NULL)
+ {
+ name_watcher = signal_data_new_take (g_steal_pointer (&name_owner_rule),
+ g_strdup (DBUS_SERVICE_DBUS),
+ g_strdup (DBUS_INTERFACE_DBUS),
+ g_strdup ("NameOwnerChanged"),
+ g_strdup (DBUS_PATH_DBUS),
+ g_strdup (sender),
+ G_DBUS_SIGNAL_FLAGS_NONE);
+ add_signal_data (connection, name_watcher, DBUS_SERVICE_DBUS);
+ }
+
+ if (name_watcher->watched_name == NULL)
+ {
+ name_watcher->watched_name = watched_name_new ();
+ name_watcher_call_get_name_owner_unlocked (connection, name_watcher);
+ }
+ else
+ {
+ g_ref_count_inc (&name_watcher->watched_name->ref_count);
+ }
+
+ signal_data->shared_name_watcher = name_watcher;
+
+ g_clear_pointer (&name_owner_rule, g_free);
+ }
+
add_signal_data (connection, signal_data, sender_unique_name);
out:
@@ -3683,10 +3991,18 @@ remove_signal_data_if_unused (GDBusConnection *connection,
const gchar *sender_unique_name;
GPtrArray *signal_data_array;
+ /* Cannot remove while there are still subscribers */
if (signal_data->subscribers->len != 0)
return;
- if (signal_data->sender_is_its_own_owner)
+ /* Cannot remove while another SignalData is still using this one
+ * as its shared_name_watcher, which holds watched_name->ref_count > 0 */
+ if (signal_data->watched_name != NULL)
+ return;
+
+ /* Point of no return: we have committed to removing it */
+
+ if (signal_data->sender != NULL && signal_data->shared_name_watcher == NULL)
sender_unique_name = signal_data->sender;
else
sender_unique_name = "";
@@ -3719,6 +4035,15 @@ remove_signal_data_if_unused (GDBusConnection *connection,
remove_match_rule (connection, signal_data->rule);
}
+ if (signal_data->shared_name_watcher != NULL)
+ {
+ SignalData *name_watcher = g_steal_pointer (&signal_data->shared_name_watcher);
+
+ name_watcher_unref_watched_name (connection, name_watcher);
+ /* May free signal_data */
+ remove_signal_data_if_unused (connection, name_watcher);
+ }
+
signal_data_free (signal_data);
}
@@ -3991,6 +4316,17 @@ schedule_callbacks (GDBusConnection *connection,
continue;
}
+ if (signal_data->watched_name != NULL)
+ {
+ /* Invariant: SignalData should only have a watched_name if it
+ * represents the NameOwnerChanged signal */
+ g_assert (g_strcmp0 (sender, DBUS_SERVICE_DBUS) == 0);
+ g_assert (g_strcmp0 (interface, DBUS_INTERFACE_DBUS) == 0);
+ g_assert (g_strcmp0 (path, DBUS_PATH_DBUS) == 0);
+ g_assert (g_strcmp0 (member, "NameOwnerChanged") == 0);
+ name_watcher_deliver_name_owner_changed_unlocked (signal_data, message);
+ }
+
for (m = 0; m < signal_data->subscribers->len; m++)
{
SignalSubscriber *subscriber = signal_data->subscribers->pdata[m];
@@ -4062,7 +4398,7 @@ distribute_signals (GDBusConnection *connection,
schedule_callbacks (connection, signal_data_array, message, sender);
}
- /* collect subscribers not matching on sender */
+ /* collect subscribers not matching on sender, or matching a well-known name */
signal_data_array = g_hash_table_lookup (connection->map_sender_unique_name_to_signal_data_array, "");
if (signal_data_array != NULL)
schedule_callbacks (connection, signal_data_array, message, sender);
--
GitLab
