From 10e9a917be7fb92b6b27837ef7a7f1d0be6095d5 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 28 Nov 2023 12:58:20 +0000 Subject: [PATCH 1/2] gdbusmessage: Cache the arg0 value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Technically we can’t rely on it being kept alive by the `message->body` pointer, unless we can guarantee that the `GVariant` is always serialised. That’s not necessarily the case, so keep a separate ref on the arg0 value at all times. This avoids a potential use-after-free. Spotted by Thomas Haller in https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3720#note_1924707. Signed-off-by: Philip Withnall Helps: #3183 --- gio/gdbusmessage.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/gio/gdbusmessage.c b/gio/gdbusmessage.c index 53488ffce..fc68f21c7 100644 --- a/gio/gdbusmessage.c +++ b/gio/gdbusmessage.c @@ -498,6 +498,7 @@ struct _GDBusMessage guint32 serial; GHashTable *headers; GVariant *body; + GVariant *arg0_cache; /* (nullable) (owned) */ #ifdef G_OS_UNIX GUnixFDList *fd_list; #endif @@ -520,6 +521,7 @@ g_dbus_message_finalize (GObject *object) g_hash_table_unref (message->headers); if (message->body != NULL) g_variant_unref (message->body); + g_clear_pointer (&message->arg0_cache, g_variant_unref); #ifdef G_OS_UNIX if (message->fd_list != NULL) g_object_unref (message->fd_list); @@ -1158,6 +1160,7 @@ g_dbus_message_set_body (GDBusMessage *message, if (body == NULL) { message->body = NULL; + message->arg0_cache = NULL; g_dbus_message_set_signature (message, NULL); } else @@ -1168,6 +1171,12 @@ g_dbus_message_set_body (GDBusMessage *message, message->body = g_variant_ref_sink (body); + if (g_variant_is_of_type (message->body, G_VARIANT_TYPE_TUPLE) && + g_variant_n_children (message->body) > 0) + message->arg0_cache = g_variant_get_child_value (message->body, 0); + else + message->arg0_cache = NULL; + type_string = g_variant_get_type_string (body); type_string_len = strlen (type_string); g_assert (type_string_len >= 2); @@ -2352,6 +2361,14 @@ g_dbus_message_new_from_blob (guchar *blob, 2, &local_error); g_variant_type_free (variant_type); + + if (message->body != NULL && + g_variant_is_of_type (message->body, G_VARIANT_TYPE_TUPLE) && + g_variant_n_children (message->body) > 0) + message->arg0_cache = g_variant_get_child_value (message->body, 0); + else + message->arg0_cache = NULL; + if (message->body == NULL) goto fail; } @@ -3391,22 +3408,13 @@ g_dbus_message_set_signature (GDBusMessage *message, const gchar * g_dbus_message_get_arg0 (GDBusMessage *message) { - const gchar *ret; - g_return_val_if_fail (G_IS_DBUS_MESSAGE (message), NULL); - ret = NULL; + if (message->arg0_cache != NULL && + g_variant_is_of_type (message->arg0_cache, G_VARIANT_TYPE_STRING)) + return g_variant_get_string (message->arg0_cache, NULL); - if (message->body != NULL && g_variant_is_of_type (message->body, G_VARIANT_TYPE_TUPLE)) - { - GVariant *item; - item = g_variant_get_child_value (message->body, 0); - if (g_variant_is_of_type (item, G_VARIANT_TYPE_STRING)) - ret = g_variant_get_string (item, NULL); - g_variant_unref (item); - } - - return ret; + return NULL; } /* ---------------------------------------------------------------------------------------------------- */ @@ -3849,6 +3857,7 @@ g_dbus_message_copy (GDBusMessage *message, * to just ref (as opposed to deep-copying) the GVariant instances */ ret->body = message->body != NULL ? g_variant_ref (message->body) : NULL; + ret->arg0_cache = message->arg0_cache != NULL ? g_variant_ref (message->arg0_cache) : NULL; g_hash_table_iter_init (&iter, message->headers); while (g_hash_table_iter_next (&iter, &header_key, (gpointer) &header_value)) g_hash_table_insert (ret->headers, header_key, g_variant_ref (header_value)); From 96c317418ae2c85771a85b2efebfb0de74cfa6b5 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 22 Nov 2023 12:40:07 +0000 Subject: [PATCH 2/2] gdbusconnection: Support matching object paths with arg0 matching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GDBus has always supported matching strings with arg0 matching (`G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH`). In D-Bus 1.5, support was added to the spec for also matching object paths. This got forgotten about and was never added to GDBus, meaning that `G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH` won’t match against a signal arg0 of type `o`. Fix that, and add a unit test. To do so, we need to add a new `g_dbus_message_get_arg0_path()` API to complement the existing `g_dbus_message_get_arg0()` API. The approach of letting `g_dbus_message_get_arg0()` return an object-path *or* a string would not work, as it’s also called in the implementation of `G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE`, which must only match string-typed arg0 values and not object-path-typed ones. Signed-off-by: Philip Withnall Fixes: #3183 --- gio/gdbusconnection.c | 14 +++++++++----- gio/gdbusmessage.c | 28 +++++++++++++++++++++++++++ gio/gdbusmessage.h | 3 ++- gio/tests/gdbus-connection.c | 37 +++++++++++++++++++++--------------- 4 files changed, 61 insertions(+), 21 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 09f62d79e..3e17a5d21 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -3867,16 +3867,22 @@ schedule_callbacks (GDBusConnection *connection, const gchar *member; const gchar *path; const gchar *arg0; + const gchar *arg0_path; interface = NULL; member = NULL; path = NULL; arg0 = NULL; + arg0_path = NULL; interface = g_dbus_message_get_interface (message); member = g_dbus_message_get_member (message); path = g_dbus_message_get_path (message); arg0 = g_dbus_message_get_arg0 (message); + arg0_path = g_dbus_message_get_arg0_path (message); + + /* These two are mutually exclusive through the type system. */ + g_assert (arg0 == NULL || arg0_path == NULL); #if 0 g_print ("In schedule_callbacks:\n" @@ -3910,17 +3916,15 @@ schedule_callbacks (GDBusConnection *connection, if (signal_data->arg0 != NULL) { - if (arg0 == NULL) - continue; - if (signal_data->flags & G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE) { - if (!namespace_rule_matches (signal_data->arg0, arg0)) + if (arg0 == NULL || !namespace_rule_matches (signal_data->arg0, arg0)) continue; } else if (signal_data->flags & G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH) { - if (!path_rule_matches (signal_data->arg0, arg0)) + if ((arg0 == NULL || !path_rule_matches (signal_data->arg0, arg0)) && + (arg0_path == NULL || !path_rule_matches (signal_data->arg0, arg0_path))) continue; } else if (!g_str_equal (signal_data->arg0, arg0)) diff --git a/gio/gdbusmessage.c b/gio/gdbusmessage.c index fc68f21c7..fa6077f86 100644 --- a/gio/gdbusmessage.c +++ b/gio/gdbusmessage.c @@ -3400,6 +3400,9 @@ g_dbus_message_set_signature (GDBusMessage *message, * * Convenience to get the first item in the body of @message. * + * See [method@Gio.DBusMessage.get_arg0_path] for returning object-path-typed + * arg0 values. + * * Returns: (nullable): The string item or %NULL if the first item in the body of * @message is not a string. * @@ -3417,6 +3420,31 @@ g_dbus_message_get_arg0 (GDBusMessage *message) return NULL; } +/** + * g_dbus_message_get_arg0_path: + * @message: A `GDBusMessage`. + * + * Convenience to get the first item in the body of @message. + * + * See [method@Gio.DBusMessage.get_arg0] for returning string-typed arg0 values. + * + * Returns: (nullable): The object path item or `NULL` if the first item in the + * body of @message is not an object path. + * + * Since: 2.80 + */ +const gchar * +g_dbus_message_get_arg0_path (GDBusMessage *message) +{ + g_return_val_if_fail (G_IS_DBUS_MESSAGE (message), NULL); + + if (message->arg0_cache != NULL && + g_variant_is_of_type (message->arg0_cache, G_VARIANT_TYPE_OBJECT_PATH)) + return g_variant_get_string (message->arg0_cache, NULL); + + return NULL; +} + /* ---------------------------------------------------------------------------------------------------- */ /** diff --git a/gio/gdbusmessage.h b/gio/gdbusmessage.h index 6e4bb9e8e..643aca074 100644 --- a/gio/gdbusmessage.h +++ b/gio/gdbusmessage.h @@ -176,7 +176,8 @@ void g_dbus_message_set_num_unix_fds (GDBusMessage GIO_AVAILABLE_IN_ALL const gchar *g_dbus_message_get_arg0 (GDBusMessage *message); - +GIO_AVAILABLE_IN_2_80 +const gchar *g_dbus_message_get_arg0_path (GDBusMessage *message); GIO_AVAILABLE_IN_ALL GDBusMessage *g_dbus_message_new_from_blob (guchar *blob, diff --git a/gio/tests/gdbus-connection.c b/gio/tests/gdbus-connection.c index 05b1f120a..7909123c3 100644 --- a/gio/tests/gdbus-connection.c +++ b/gio/tests/gdbus-connection.c @@ -742,6 +742,7 @@ test_match_rule (GDBusConnection *connection, GDBusSignalFlags flags, gchar *arg0_rule, gchar *arg0, + const gchar *signal_type, gboolean should_match) { guint subscription_ids[2]; @@ -766,7 +767,7 @@ test_match_rule (GDBusConnection *connection, g_dbus_connection_emit_signal (connection, NULL, "/", "org.gtk.ExampleInterface", - "Foo", g_variant_new ("(s)", arg0), + "Foo", g_variant_new (signal_type, arg0), &error); g_assert_no_error (error); @@ -793,22 +794,28 @@ test_connection_signal_match_rules (void) session_bus_up (); con = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_NONE, "foo", "foo", TRUE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_NONE, "foo", "bar", FALSE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_NONE, "foo", "foo", "(s)", TRUE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_NONE, "foo", "bar", "(s)", FALSE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, "org.gtk", "", FALSE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, "org.gtk", "org", FALSE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, "org.gtk", "org.gtk", TRUE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, "org.gtk", "org.gtk.Example", TRUE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, "org.gtk", "org.gtk+", FALSE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, "org.gtk", "", "(s)", FALSE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, "org.gtk", "org", "(s)", FALSE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, "org.gtk", "org.gtk", "(s)", TRUE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, "org.gtk", "org.gtk.Example", "(s)", TRUE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, "org.gtk", "org.gtk+", "(s)", FALSE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/", "/", TRUE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/", "", FALSE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/Example", "/org/gtk/Example", TRUE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/", "/org/gtk/Example", TRUE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/Example", "/org/gtk/", TRUE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/Example", "/org/gtk", FALSE); - test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk+", "/org/gtk", FALSE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/", "/", "(s)", TRUE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/", "", "(s)", FALSE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/Example", "/org/gtk/Example", "(s)", TRUE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/", "/org/gtk/Example", "(s)", TRUE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/Example", "/org/gtk/", "(s)", TRUE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/Example", "/org/gtk", "(s)", FALSE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk+", "/org/gtk", "(s)", FALSE); + + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/", "/", "(o)", TRUE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/Example", "/org/gtk/Example", "(o)", TRUE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/", "/org/gtk/Example", "(o)", TRUE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk/Example", "/org/gtk", "(o)", FALSE); + test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/org/gtk+", "/org/gtk", "(o)", FALSE); g_object_unref (con); session_bus_down ();