From df2c5d925ac4b8f1708bafa5ac1d35acada05d55 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 15 May 2024 12:26:36 +0100 Subject: [PATCH 1/2] gmenuexporter: Fix a NULL pointer dereference on an error handling path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This latent bug wasn’t triggered until commit 3f30ec86c (or its cherry-pick onto `glib-2-80`, 747e3af99, which was first released in 2.80.1). That change means that `g_menu_exporter_free()` is now called on the registration failure path by `g_dbus_connection_register_object()` before it returns. The caller then tries to call `g_slice_free()` on the exporter again. The call to `g_menu_exporter_free()` tries to dereference/free members of the exporter which it expects to be initialised — but because this is happening in an error handling path, they are not initialised. If it were to get any further, the `g_slice_free()` would then be a double-free on the exporter allocation. Fix that by making `g_menu_exporter_free()` robust to some of the exporter members being `NULL`, and moving some of the initialisation code higher in `g_dbus_connection_export_menu_model()`, and removing the duplicate free code on the error handling path. This includes a unit test. Signed-off-by: Philip Withnall Fixes: #3366 --- gio/gmenuexporter.c | 23 ++++++++--------------- gio/tests/gmenumodel.c | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 15 deletions(-) diff --git a/gio/gmenuexporter.c b/gio/gmenuexporter.c index 909780cb2c..1d4db13523 100644 --- a/gio/gmenuexporter.c +++ b/gio/gmenuexporter.c @@ -707,11 +707,9 @@ g_menu_exporter_create_group (GMenuExporter *exporter) } static void -g_menu_exporter_free (gpointer user_data) +g_menu_exporter_free (GMenuExporter *exporter) { - GMenuExporter *exporter = user_data; - - g_menu_exporter_menu_free (exporter->root); + g_clear_pointer (&exporter->root, g_menu_exporter_menu_free); g_clear_pointer (&exporter->peer_remote, g_menu_exporter_remote_free); g_hash_table_unref (exporter->remotes); g_hash_table_unref (exporter->groups); @@ -794,21 +792,16 @@ g_dbus_connection_export_menu_model (GDBusConnection *connection, guint id; exporter = g_slice_new0 (GMenuExporter); - - id = g_dbus_connection_register_object (connection, object_path, org_gtk_Menus_get_interface (), - &vtable, exporter, g_menu_exporter_free, error); - - if (id == 0) - { - g_slice_free (GMenuExporter, exporter); - return 0; - } - exporter->connection = g_object_ref (connection); exporter->object_path = g_strdup (object_path); exporter->groups = g_hash_table_new (NULL, NULL); exporter->remotes = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_menu_exporter_remote_free); - exporter->root = g_menu_exporter_group_add_menu (g_menu_exporter_create_group (exporter), menu); + + id = g_dbus_connection_register_object (connection, object_path, org_gtk_Menus_get_interface (), + &vtable, exporter, (GDestroyNotify) g_menu_exporter_free, error); + + if (id != 0) + exporter->root = g_menu_exporter_group_add_menu (g_menu_exporter_create_group (exporter), menu); return id; } diff --git a/gio/tests/gmenumodel.c b/gio/tests/gmenumodel.c index d75f36a297..22d1b4d61e 100644 --- a/gio/tests/gmenumodel.c +++ b/gio/tests/gmenumodel.c @@ -1159,6 +1159,42 @@ test_dbus_peer_subscriptions (void) #endif } +static void +test_dbus_export_error_handling (void) +{ + GRand *rand = NULL; + RandomMenu *menu = NULL; + GDBusConnection *bus; + GError *local_error = NULL; + guint id1, id2; + + g_test_summary ("Test that error handling of menu model export failure works"); + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/3366"); + + bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); + + rand = g_rand_new_with_seed (g_test_rand_int ()); + menu = random_menu_new (rand, 2); + + id1 = g_dbus_connection_export_menu_model (bus, "/", G_MENU_MODEL (menu), &local_error); + g_assert_no_error (local_error); + g_assert_cmpuint (id1, !=, 0); + + /* Trigger a failure by trying to export on a path which is already in use */ + id2 = g_dbus_connection_export_menu_model (bus, "/", G_MENU_MODEL (menu), &local_error); + g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_EXISTS); + g_assert_cmpuint (id2, ==, 0); + g_clear_error (&local_error); + + g_dbus_connection_unexport_menu_model (bus, id1); + + while (g_main_context_iteration (NULL, FALSE)); + + g_clear_object (&menu); + g_rand_free (rand); + g_clear_object (&bus); +} + static gpointer do_modify (gpointer data) { @@ -1658,6 +1694,7 @@ main (int argc, char **argv) g_test_add_func ("/gmenu/dbus/threaded", test_dbus_threaded); g_test_add_func ("/gmenu/dbus/peer/roundtrip", test_dbus_peer_roundtrip); g_test_add_func ("/gmenu/dbus/peer/subscriptions", test_dbus_peer_subscriptions); + g_test_add_func ("/gmenu/dbus/export/error-handling", test_dbus_export_error_handling); g_test_add_func ("/gmenu/attributes", test_attributes); g_test_add_func ("/gmenu/attributes/iterate", test_attribute_iter); g_test_add_func ("/gmenu/links", test_links); -- GitLab From 7a7137838e79e5a98e6f4eab6898e2a0dc6392cd Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 15 May 2024 14:00:09 +0100 Subject: [PATCH 2/2] gactiongroupexporter: Fix memory problems on an error handling path Almost identically to the previous commit, fix a similar latent bug in `g_dbus_connection_export_action_group()`, which was not ready to handle the fledgling `GActionGroupExporter` being freed early on an error handling path. See the previous commit message for details of the approach. This includes a unit test. Signed-off-by: Philip Withnall Fixes: #3366 --- gio/gactiongroupexporter.c | 35 ++++++++++++++------------------ gio/tests/actions.c | 41 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 20 deletions(-) diff --git a/gio/gactiongroupexporter.c b/gio/gactiongroupexporter.c index 3ec1db224e..1e253ec88b 100644 --- a/gio/gactiongroupexporter.c +++ b/gio/gactiongroupexporter.c @@ -531,10 +531,8 @@ org_gtk_Actions_method_call (GDBusConnection *connection, } static void -g_action_group_exporter_free (gpointer user_data) +g_action_group_exporter_free (GActionGroupExporter *exporter) { - GActionGroupExporter *exporter = user_data; - g_signal_handlers_disconnect_by_func (exporter->action_group, g_action_group_exporter_action_added, exporter); g_signal_handlers_disconnect_by_func (exporter->action_group, @@ -616,15 +614,6 @@ g_dbus_connection_export_action_group (GDBusConnection *connection, } exporter = g_slice_new (GActionGroupExporter); - id = g_dbus_connection_register_object (connection, object_path, org_gtk_Actions, &vtable, - exporter, g_action_group_exporter_free, error); - - if (id == 0) - { - g_slice_free (GActionGroupExporter, exporter); - return 0; - } - exporter->context = g_main_context_ref_thread_default (); exporter->pending_changes = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL); exporter->pending_source = NULL; @@ -632,14 +621,20 @@ g_dbus_connection_export_action_group (GDBusConnection *connection, exporter->connection = g_object_ref (connection); exporter->object_path = g_strdup (object_path); - g_signal_connect (action_group, "action-added", - G_CALLBACK (g_action_group_exporter_action_added), exporter); - g_signal_connect (action_group, "action-removed", - G_CALLBACK (g_action_group_exporter_action_removed), exporter); - g_signal_connect (action_group, "action-state-changed", - G_CALLBACK (g_action_group_exporter_action_state_changed), exporter); - g_signal_connect (action_group, "action-enabled-changed", - G_CALLBACK (g_action_group_exporter_action_enabled_changed), exporter); + id = g_dbus_connection_register_object (connection, object_path, org_gtk_Actions, &vtable, + exporter, (GDestroyNotify) g_action_group_exporter_free, error); + + if (id != 0) + { + g_signal_connect (action_group, "action-added", + G_CALLBACK (g_action_group_exporter_action_added), exporter); + g_signal_connect (action_group, "action-removed", + G_CALLBACK (g_action_group_exporter_action_removed), exporter); + g_signal_connect (action_group, "action-state-changed", + G_CALLBACK (g_action_group_exporter_action_state_changed), exporter); + g_signal_connect (action_group, "action-enabled-changed", + G_CALLBACK (g_action_group_exporter_action_enabled_changed), exporter); + } return id; } diff --git a/gio/tests/actions.c b/gio/tests/actions.c index a24c52c5e4..2b7a100fcf 100644 --- a/gio/tests/actions.c +++ b/gio/tests/actions.c @@ -1125,6 +1125,46 @@ test_dbus_export (void) session_bus_down (); } +static void +test_dbus_export_error_handling (void) +{ + GDBusConnection *bus = NULL; + GSimpleActionGroup *group = NULL; + GError *local_error = NULL; + guint id1, id2; + + g_test_summary ("Test that error handling of action group export failure works"); + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/3366"); + + session_bus_up (); + bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); + + group = g_simple_action_group_new (); + g_simple_action_group_add_entries (group, + exported_entries, + G_N_ELEMENTS (exported_entries), + NULL); + + id1 = g_dbus_connection_export_action_group (bus, "/", G_ACTION_GROUP (group), &local_error); + g_assert_no_error (local_error); + g_assert_cmpuint (id1, !=, 0); + + /* Trigger a failure by trying to export on a path which is already in use */ + id2 = g_dbus_connection_export_action_group (bus, "/", G_ACTION_GROUP (group), &local_error); + g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_EXISTS); + g_assert_cmpuint (id2, ==, 0); + g_clear_error (&local_error); + + g_dbus_connection_unexport_action_group (bus, id1); + + while (g_main_context_iteration (NULL, FALSE)); + + g_object_unref (group); + g_object_unref (bus); + + session_bus_down (); +} + static gpointer do_export (gpointer data) { @@ -1448,6 +1488,7 @@ main (int argc, char **argv) g_test_add_func ("/actions/entries", test_entries); g_test_add_func ("/actions/parse-detailed", test_parse_detailed); g_test_add_func ("/actions/dbus/export", test_dbus_export); + g_test_add_func ("/actions/dbus/export/error-handling", test_dbus_export_error_handling); g_test_add_func ("/actions/dbus/threaded", test_dbus_threaded); g_test_add_func ("/actions/dbus/bug679509", test_bug679509); g_test_add_func ("/actions/property", test_property_actions); -- GitLab