From 310f2c1632e05c4f32be033c009642012741d876 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 24 Sep 2021 08:28:19 +0100 Subject: [PATCH 1/6] gdbusconnection: Move ExportedSubtree definition Move it further up the file, but make no changes to it. This will help with a subsequent commit. Signed-off-by: Philip Withnall Helps: #2400 --- gio/gdbusconnection.c | 54 +++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index d730111f8..24a50fcf2 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -4113,6 +4113,33 @@ exported_interface_free (ExportedInterface *ei) g_free (ei); } +struct ExportedSubtree +{ + guint id; + gchar *object_path; + GDBusConnection *connection; + GDBusSubtreeVTable *vtable; + GDBusSubtreeFlags flags; + + GMainContext *context; + gpointer user_data; + GDestroyNotify user_data_free_func; +}; + +static void +exported_subtree_free (ExportedSubtree *es) +{ + call_destroy_notify (es->context, + es->user_data_free_func, + es->user_data); + + g_main_context_unref (es->context); + + _g_dbus_subtree_vtable_free (es->vtable); + g_free (es->object_path); + g_free (es); +} + /* ---------------------------------------------------------------------------------------------------- */ /* Convenience function to check if @registration_id (if not zero) or @@ -6401,33 +6428,6 @@ g_dbus_connection_call_with_unix_fd_list_sync (GDBusConnection *connection, /* ---------------------------------------------------------------------------------------------------- */ -struct ExportedSubtree -{ - guint id; - gchar *object_path; - GDBusConnection *connection; - GDBusSubtreeVTable *vtable; - GDBusSubtreeFlags flags; - - GMainContext *context; - gpointer user_data; - GDestroyNotify user_data_free_func; -}; - -static void -exported_subtree_free (ExportedSubtree *es) -{ - call_destroy_notify (es->context, - es->user_data_free_func, - es->user_data); - - g_main_context_unref (es->context); - - _g_dbus_subtree_vtable_free (es->vtable); - g_free (es->object_path); - g_free (es); -} - /* called without lock held in the thread where the caller registered * the subtree */ From a497fdf302bf67e4df2e1474389c0ff2152f1e99 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 24 Sep 2021 08:58:42 +0100 Subject: [PATCH 2/6] gdbusconnection: Add some ownership annotations Signed-off-by: Philip Withnall --- gio/gdbusconnection.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 24a50fcf2..40ce1b6fc 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -4086,11 +4086,11 @@ typedef struct ExportedObject *eo; guint id; - gchar *interface_name; - GDBusInterfaceVTable *vtable; - GDBusInterfaceInfo *interface_info; + gchar *interface_name; /* (owned) */ + GDBusInterfaceVTable *vtable; /* (owned) */ + GDBusInterfaceInfo *interface_info; /* (owned) */ - GMainContext *context; + GMainContext *context; /* (owned) */ gpointer user_data; GDestroyNotify user_data_free_func; } ExportedInterface; @@ -4116,12 +4116,12 @@ exported_interface_free (ExportedInterface *ei) struct ExportedSubtree { guint id; - gchar *object_path; - GDBusConnection *connection; - GDBusSubtreeVTable *vtable; + gchar *object_path; /* (owned) */ + GDBusConnection *connection; /* (unowned) */ + GDBusSubtreeVTable *vtable; /* (owned) */ GDBusSubtreeFlags flags; - GMainContext *context; + GMainContext *context; /* (owned) */ gpointer user_data; GDestroyNotify user_data_free_func; }; From c8c2ed4af5cdf8d77af2cd9a2a4b4f6ac8d1fc70 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 24 Sep 2021 09:03:40 +0100 Subject: [PATCH 3/6] gdbusconnection: Make ExportedInterface/ExportedSubtree refcounted This is needed for an upcoming change which decouples their lifecycle from their presence in the `map_id_to_ei` and `map_id_to_es` hash tables. Signed-off-by: Philip Withnall Helps: #2400 --- gio/gdbusconnection.c | 46 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 40ce1b6fc..71913c1ec 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -466,7 +466,8 @@ typedef struct ExportedObject ExportedObject; static void exported_object_free (ExportedObject *eo); typedef struct ExportedSubtree ExportedSubtree; -static void exported_subtree_free (ExportedSubtree *es); +static ExportedSubtree *exported_subtree_ref (ExportedSubtree *es); +static void exported_subtree_unref (ExportedSubtree *es); enum { @@ -1096,7 +1097,7 @@ g_dbus_connection_init (GDBusConnection *connection) connection->map_object_path_to_es = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, - (GDestroyNotify) exported_subtree_free); + (GDestroyNotify) exported_subtree_unref); connection->map_id_to_es = g_hash_table_new (g_direct_hash, g_direct_equal); @@ -4085,6 +4086,8 @@ typedef struct { ExportedObject *eo; + gint refcount; /* (atomic) */ + guint id; gchar *interface_name; /* (owned) */ GDBusInterfaceVTable *vtable; /* (owned) */ @@ -4095,10 +4098,21 @@ typedef struct GDestroyNotify user_data_free_func; } ExportedInterface; -/* called with lock held */ -static void -exported_interface_free (ExportedInterface *ei) +static ExportedInterface * +exported_interface_ref (ExportedInterface *ei) { + g_atomic_int_inc (&ei->refcount); + + return ei; +} + +/* May be called with lock held */ +static void +exported_interface_unref (ExportedInterface *ei) +{ + if (!g_atomic_int_dec_and_test (&ei->refcount)) + return; + g_dbus_interface_info_cache_release (ei->interface_info); g_dbus_interface_info_unref ((GDBusInterfaceInfo *) ei->interface_info); @@ -4115,6 +4129,8 @@ exported_interface_free (ExportedInterface *ei) struct ExportedSubtree { + gint refcount; /* (atomic) */ + guint id; gchar *object_path; /* (owned) */ GDBusConnection *connection; /* (unowned) */ @@ -4126,9 +4142,21 @@ struct ExportedSubtree GDestroyNotify user_data_free_func; }; -static void -exported_subtree_free (ExportedSubtree *es) +static ExportedSubtree * +exported_subtree_ref (ExportedSubtree *es) { + g_atomic_int_inc (&es->refcount); + + return es; +} + +/* May be called with lock held */ +static void +exported_subtree_unref (ExportedSubtree *es) +{ + if (!g_atomic_int_dec_and_test (&es->refcount)) + return; + call_destroy_notify (es->context, es->user_data_free_func, es->user_data); @@ -5251,7 +5279,7 @@ g_dbus_connection_register_object (GDBusConnection *connection, eo->map_if_name_to_ei = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, - (GDestroyNotify) exported_interface_free); + (GDestroyNotify) exported_interface_unref); g_hash_table_insert (connection->map_object_path_to_eo, eo->object_path, eo); } @@ -5268,6 +5296,7 @@ g_dbus_connection_register_object (GDBusConnection *connection, } ei = g_new0 (ExportedInterface, 1); + ei->refcount = 1; ei->id = (guint) g_atomic_int_add (&_global_registration_id, 1); /* TODO: overflow etc. */ ei->eo = eo; ei->user_data = user_data; @@ -6924,6 +6953,7 @@ g_dbus_connection_register_subtree (GDBusConnection *connection, } es = g_new0 (ExportedSubtree, 1); + es->refcount = 1; es->object_path = g_strdup (object_path); es->connection = connection; From 50fbf05d61db500df9052bb682d9c01e0bf51ffb Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 24 Sep 2021 10:52:41 +0100 Subject: [PATCH 4/6] gdbusconnection: Fix race between method calls and object unregistration If `g_dbus_connection_unregister_object()` (or `unregister_subtree()`) was called from one thread, while an idle callback for a method call (or a property get or set) was being invoked in another, it was possible for the two to race after the idle callback had checked that the object/subtree was registered, but before it had finished dereferencing all the data related to that object/subtree. Unregistering the object/subtree would immediately free the data, leading the idle callback to cause a use-after-free error. Fix that by giving the idle callback a strong reference to the data from inside the locked section where it checks whether the object/subtree is still registered. Signed-off-by: Philip Withnall Fixes: #2400 --- gio/gdbusconnection.c | 66 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 71913c1ec..e6c0b70b4 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -4116,6 +4116,9 @@ exported_interface_unref (ExportedInterface *ei) g_dbus_interface_info_cache_release (ei->interface_info); g_dbus_interface_info_unref ((GDBusInterfaceInfo *) ei->interface_info); + /* All uses of ei->vtable from callbacks scheduled in idle functions must + * have completed by this call_destroy_notify() call, as language bindings + * may destroy function closures in this callback. */ call_destroy_notify (ei->context, ei->user_data_free_func, ei->user_data); @@ -4157,6 +4160,9 @@ exported_subtree_unref (ExportedSubtree *es) if (!g_atomic_int_dec_and_test (&es->refcount)) return; + /* All uses of es->vtable from callbacks scheduled in idle functions must + * have completed by this call_destroy_notify() call, as language bindings + * may destroy function closures in this callback. */ call_destroy_notify (es->context, es->user_data_free_func, es->user_data); @@ -4174,30 +4180,45 @@ exported_subtree_unref (ExportedSubtree *es) * @subtree_registration_id (if not zero) has been unregistered. If * so, returns %TRUE. * + * If not, sets @out_ei and/or @out_es to a strong reference to the relevant + * #ExportedInterface/#ExportedSubtree and returns %FALSE. + * * May be called by any thread. Caller must *not* hold lock. */ static gboolean -has_object_been_unregistered (GDBusConnection *connection, - guint registration_id, - guint subtree_registration_id) +has_object_been_unregistered (GDBusConnection *connection, + guint registration_id, + ExportedInterface **out_ei, + guint subtree_registration_id, + ExportedSubtree **out_es) { gboolean ret; + ExportedInterface *ei = NULL; + gpointer es = NULL; g_return_val_if_fail (G_IS_DBUS_CONNECTION (connection), FALSE); ret = FALSE; CONNECTION_LOCK (connection); - if (registration_id != 0 && g_hash_table_lookup (connection->map_id_to_ei, - GUINT_TO_POINTER (registration_id)) == NULL) + + if (registration_id != 0) { - ret = TRUE; + ei = g_hash_table_lookup (connection->map_id_to_ei, GUINT_TO_POINTER (registration_id)); + if (ei == NULL) + ret = TRUE; + else if (out_ei != NULL) + *out_ei = exported_interface_ref (ei); } - else if (subtree_registration_id != 0 && g_hash_table_lookup (connection->map_id_to_es, - GUINT_TO_POINTER (subtree_registration_id)) == NULL) + if (subtree_registration_id != 0) { - ret = TRUE; + es = g_hash_table_lookup (connection->map_id_to_es, GUINT_TO_POINTER (subtree_registration_id)); + if (es == NULL) + ret = TRUE; + else if (out_es != NULL) + *out_es = exported_subtree_ref (es); } + CONNECTION_UNLOCK (connection); return ret; @@ -4234,10 +4255,14 @@ invoke_get_property_in_idle_cb (gpointer _data) GVariant *value; GError *error; GDBusMessage *reply; + ExportedInterface *ei = NULL; + ExportedSubtree *es = NULL; if (has_object_been_unregistered (data->connection, data->registration_id, - data->subtree_registration_id)) + &ei, + data->subtree_registration_id, + &es)) { reply = g_dbus_message_new_method_error (data->message, "org.freedesktop.DBus.Error.UnknownMethod", @@ -4284,6 +4309,9 @@ invoke_get_property_in_idle_cb (gpointer _data) } out: + g_clear_pointer (&ei, exported_interface_unref); + g_clear_pointer (&es, exported_subtree_unref); + return FALSE; } @@ -4581,10 +4609,14 @@ invoke_get_all_properties_in_idle_cb (gpointer _data) GVariantBuilder builder; GDBusMessage *reply; guint n; + ExportedInterface *ei = NULL; + ExportedSubtree *es = NULL; if (has_object_been_unregistered (data->connection, data->registration_id, - data->subtree_registration_id)) + &ei, + data->subtree_registration_id, + &es)) { reply = g_dbus_message_new_method_error (data->message, "org.freedesktop.DBus.Error.UnknownMethod", @@ -4637,6 +4669,9 @@ invoke_get_all_properties_in_idle_cb (gpointer _data) g_object_unref (reply); out: + g_clear_pointer (&ei, exported_interface_unref); + g_clear_pointer (&es, exported_subtree_unref); + return FALSE; } @@ -4946,13 +4981,17 @@ call_in_idle_cb (gpointer user_data) GDBusInterfaceVTable *vtable; guint registration_id; guint subtree_registration_id; + ExportedInterface *ei = NULL; + ExportedSubtree *es = NULL; registration_id = GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (invocation), "g-dbus-registration-id")); subtree_registration_id = GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (invocation), "g-dbus-subtree-registration-id")); if (has_object_been_unregistered (g_dbus_method_invocation_get_connection (invocation), registration_id, - subtree_registration_id)) + &ei, + subtree_registration_id, + &es)) { GDBusMessage *reply; reply = g_dbus_message_new_method_error (g_dbus_method_invocation_get_message (invocation), @@ -4978,6 +5017,9 @@ call_in_idle_cb (gpointer user_data) g_dbus_method_invocation_get_user_data (invocation)); out: + g_clear_pointer (&ei, exported_interface_unref); + g_clear_pointer (&es, exported_subtree_unref); + return FALSE; } From 117b748e44e0ec930fcb9641e3f808572d4a41f2 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 24 Sep 2021 10:55:10 +0100 Subject: [PATCH 5/6] gdbusconnection: Fix race between subtree method call and unregistration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix another variant of the previous commit, this time specific to the idle callback of a method call on a subtree object, racing with unregistration of that subtree. In this case, the `process_subtree_vtable_message_in_idle_cb()` idle callback already has a pointer to the right `ExportedSubtree` struct, but again doesn’t have a strong reference to it. Signed-off-by: Philip Withnall Helps: #2400 --- gio/gdbusconnection.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index e6c0b70b4..73b5b309a 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -6824,14 +6824,15 @@ handle_subtree_method_invocation (GDBusConnection *connection, typedef struct { - GDBusMessage *message; - ExportedSubtree *es; + GDBusMessage *message; /* (owned) */ + ExportedSubtree *es; /* (owned) */ } SubtreeDeferredData; static void subtree_deferred_data_free (SubtreeDeferredData *data) { g_object_unref (data->message); + exported_subtree_unref (data->es); g_free (data); } @@ -6890,7 +6891,7 @@ subtree_message_func (GDBusConnection *connection, data = g_new0 (SubtreeDeferredData, 1); data->message = g_object_ref (message); - data->es = es; + data->es = exported_subtree_ref (es); /* defer this call to an idle handler in the right thread */ idle_source = g_idle_source_new (); From 34ce204fd758e2ce0ab6bf152051534f46cdb336 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 24 Sep 2021 10:57:20 +0100 Subject: [PATCH 6/6] tests: Add D-Bus object/subtree unregistration tests These tests cover the fixes from the previous two commits. Signed-off-by: Philip Withnall Helps: #2400 --- gio/tests/gdbus-export.c | 180 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 180 insertions(+) diff --git a/gio/tests/gdbus-export.c b/gio/tests/gdbus-export.c index aec21d7d0..4cdc24492 100644 --- a/gio/tests/gdbus-export.c +++ b/gio/tests/gdbus-export.c @@ -1792,6 +1792,184 @@ test_async_properties (void) g_object_unref (c); } +typedef struct +{ + GDBusConnection *connection; /* (owned) */ + guint registration_id; + guint subtree_registration_id; +} ThreadedUnregistrationData; + +static gpointer +unregister_thread_cb (gpointer user_data) +{ + ThreadedUnregistrationData *data = user_data; + + /* Sleeping here makes the race more likely to be hit, as it balances the + * time taken to set up the thread and unregister, with the time taken to + * make and handle the D-Bus call. This will likely change with future kernel + * versions, but there isn’t a more deterministic synchronisation point that + * I can think of to use instead. */ + usleep (330); + + if (data->registration_id > 0) + g_assert_true (g_dbus_connection_unregister_object (data->connection, data->registration_id)); + + if (data->subtree_registration_id > 0) + g_assert_true (g_dbus_connection_unregister_subtree (data->connection, data->subtree_registration_id)); + + return NULL; +} + +static void +async_result_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data) +{ + GAsyncResult **result_out = user_data; + + *result_out = g_object_ref (result); + g_main_context_wakeup (NULL); +} + +/* Returns %TRUE if this iteration resolved the race with the unregistration + * first, %FALSE if the call handler was invoked first. */ +static gboolean +test_threaded_unregistration_iteration (gboolean subtree) +{ + ThreadedUnregistrationData data = { NULL, 0, 0 }; + ObjectRegistrationData object_registration_data = { 0, 0, 2 }; + GError *local_error = NULL; + GThread *unregister_thread = NULL; + const gchar *object_path; + GVariant *value = NULL; + const gchar *value_str; + GAsyncResult *call_result = NULL; + gboolean unregistration_was_first; + + data.connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &local_error); + g_assert_no_error (local_error); + g_assert_nonnull (data.connection); + + /* Register an object or a subtree */ + if (!subtree) + { + data.registration_id = g_dbus_connection_register_object (data.connection, + "/foo/boss", + (GDBusInterfaceInfo *) &foo_interface_info, + &foo_vtable, + &object_registration_data, + on_object_unregistered, + &local_error); + g_assert_no_error (local_error); + g_assert_cmpint (data.registration_id, >, 0); + + object_path = "/foo/boss"; + } + else + { + data.subtree_registration_id = g_dbus_connection_register_subtree (data.connection, + "/foo/boss/executives", + &subtree_vtable, + G_DBUS_SUBTREE_FLAGS_NONE, + &object_registration_data, + on_subtree_unregistered, + &local_error); + g_assert_no_error (local_error); + g_assert_cmpint (data.subtree_registration_id, >, 0); + + object_path = "/foo/boss/executives/vp0"; + } + + /* Allow the registrations to go through. */ + g_main_context_iteration (NULL, FALSE); + + /* Spawn a thread to unregister the object/subtree. This will race with + * the call we subsequently make. */ + unregister_thread = g_thread_new ("unregister-object", + unregister_thread_cb, &data); + + /* Call a method on the object (or an object in the subtree). The callback + * will be invoked in this main context. */ + g_dbus_connection_call (data.connection, + g_dbus_connection_get_unique_name (data.connection), + object_path, + "org.example.Foo", + "Method1", + g_variant_new ("(s)", "winwinwin"), + NULL, + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + async_result_cb, + &call_result); + + while (call_result == NULL) + g_main_context_iteration (NULL, TRUE); + + value = g_dbus_connection_call_finish (data.connection, call_result, &local_error); + + /* The result of the method could either be an error (that the object doesn’t + * exist) or a valid result, depending on how the thread was scheduled + * relative to the call. */ + unregistration_was_first = (value == NULL); + if (value != NULL) + { + g_assert_no_error (local_error); + g_assert_true (g_variant_is_of_type (value, G_VARIANT_TYPE ("(s)"))); + g_variant_get (value, "(&s)", &value_str); + g_assert_cmpstr (value_str, ==, "You passed the string 'winwinwin'. Jolly good!"); + } + else + { + g_assert_null (value); + g_assert_error (local_error, G_DBUS_ERROR, G_DBUS_ERROR_UNKNOWN_METHOD); + } + + g_clear_pointer (&value, g_variant_unref); + g_clear_error (&local_error); + + /* Tidy up. */ + g_thread_join (g_steal_pointer (&unregister_thread)); + + g_clear_object (&call_result); + g_clear_object (&data.connection); + + return unregistration_was_first; +} + +static void +test_threaded_unregistration (gconstpointer test_data) +{ + gboolean subtree = GPOINTER_TO_INT (test_data); + guint i; + guint n_iterations_unregistration_first = 0; + guint n_iterations_call_first = 0; + + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2400"); + g_test_summary ("Test that object/subtree unregistration from one thread " + "doesn’t cause problems when racing with method callbacks " + "in another thread for that object or subtree"); + + /* Run iterations of the test until it’s likely we’ve hit the race. Limit the + * number of iterations so the test doesn’t run forever if not. The choice of + * 100 is arbitrary. */ + for (i = 0; i < 1000 && (n_iterations_unregistration_first < 100 || n_iterations_call_first < 100); i++) + { + if (test_threaded_unregistration_iteration (subtree)) + n_iterations_unregistration_first++; + else + n_iterations_call_first++; + } + + /* If the condition below is met, we probably failed to reproduce the race. + * Don’t fail the test, though, as we can’t always control whether we hit the + * race, and spurious test failures are annoying. */ + if (n_iterations_unregistration_first < 100 || + n_iterations_call_first < 100) + g_test_skip_printf ("Failed to reproduce race (%u iterations with unregistration first, %u with call first); skipping test", + n_iterations_unregistration_first, n_iterations_call_first); +} + /* ---------------------------------------------------------------------------------------------------- */ int @@ -1809,6 +1987,8 @@ main (int argc, g_test_add_func ("/gdbus/object-registration-with-closures", test_object_registration_with_closures); g_test_add_func ("/gdbus/registered-interfaces", test_registered_interfaces); g_test_add_func ("/gdbus/async-properties", test_async_properties); + g_test_add_data_func ("/gdbus/threaded-unregistration/object", GINT_TO_POINTER (FALSE), test_threaded_unregistration); + g_test_add_data_func ("/gdbus/threaded-unregistration/subtree", GINT_TO_POINTER (TRUE), test_threaded_unregistration); /* TODO: check that we spit out correct introspection data */ /* TODO: check that registering a whole subtree works */