Bug 625584 – Crashes application on unref with signal subscription

Don't do too much work in the finalizer - in particular, there's no
need to send RemoveMatch() messages to the bus daemon since we're
going to sever the connection and the bus will garbage collect
anyway. In this case it crashed the process.

Also add a test case that checks that the appropriate GDestroyNotify
callbacks are called when unreffing a connection with either 1)
exported objects; 2) signal subscriptions or 3) filter functions
.. yes, ideally apps would unregister such callbacks before giving up
their ref but that's not how things work :-)

Signed-off-by: David Zeuthen <davidz@redhat.com>

diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 116d547..47b347f 100644 (file)
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -318,6 +318,9 @@ struct _GDBusConnection
 
   GDBusAuthObserver *authentication_observer;
   GCredentials *crendentials;
+
+  /* set to TRUE when finalizing */
+  gboolean finalizing;
 };
 
 typedef struct ExportedObject ExportedObject;
@@ -408,13 +411,19 @@ g_dbus_connection_finalize (GObject *object)
 {
   GDBusConnection *connection = G_DBUS_CONNECTION (object);
 
+  connection->finalizing = TRUE;
+
+  purge_all_signal_subscriptions (connection);
+
+  purge_all_filters (connection);
+  g_ptr_array_unref (connection->filters);
+
   if (connection->authentication_observer != NULL)
     g_object_unref (connection->authentication_observer);
 
   if (connection->auth != NULL)
     g_object_unref (connection->auth);
 
-  //g_debug ("finalizing %p", connection);
   if (connection->stream != NULL)
     {
       /* We don't really care if closing the stream succeeds or not */
@@ -437,7 +446,6 @@ g_dbus_connection_finalize (GObject *object)
 
   g_hash_table_unref (connection->map_method_serial_to_send_message_data);
 
-  purge_all_signal_subscriptions (connection);
   g_hash_table_unref (connection->map_rule_to_signal_data);
   g_hash_table_unref (connection->map_id_to_signal_data);
   g_hash_table_unref (connection->map_sender_unique_name_to_signal_data_array);
@@ -447,9 +455,6 @@ g_dbus_connection_finalize (GObject *object)
   g_hash_table_unref (connection->map_id_to_es);
   g_hash_table_unref (connection->map_object_path_to_es);
 
-  purge_all_filters (connection);
-  g_ptr_array_unref (connection->filters);
-
   if (connection->main_context_at_construction != NULL)
     g_main_context_unref (connection->main_context_at_construction);
 
@@ -3047,7 +3052,7 @@ g_dbus_connection_signal_subscribe (GDBusConnection     *connection,
 
 /* ---------------------------------------------------------------------------------------------------- */
 
-/* must hold lock when calling this */
+/* must hold lock when calling this (except if connection->finalizing is TRUE) */
 static void
 unsubscribe_id_internal (GDBusConnection *connection,
                          guint            subscription_id,
@@ -3097,7 +3102,7 @@ unsubscribe_id_internal (GDBusConnection *connection,
           if (connection->flags & G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION)
             {
               if (!is_signal_data_for_name_lost_or_acquired (signal_data))
-                if (!connection->closed)
+                if (!connection->closed && !connection->finalizing)
                   remove_match_rule (connection, signal_data->rule);
             }
           signal_data_free (signal_data);

diff --git a/gio/tests/gdbus-connection.c b/gio/tests/gdbus-connection.c
index 4b25aa3..c25bf48 100644 (file)
--- a/gio/tests/gdbus-connection.c
+++ b/gio/tests/gdbus-connection.c
@@ -32,10 +32,65 @@
 /* all tests rely on a shared mainloop */
 static GMainLoop *loop = NULL;
 
+static gboolean
+test_connection_quit_mainloop (gpointer user_data)
+{
+  gboolean *quit_mainloop_fired = user_data;
+  *quit_mainloop_fired = TRUE;
+  g_main_loop_quit (loop);
+  return TRUE;
+}
+
 /* ---------------------------------------------------------------------------------------------------- */
 /* Connection life-cycle testing */
 /* ---------------------------------------------------------------------------------------------------- */
 
+static const GDBusInterfaceInfo boo_interface_info =
+{
+  -1,
+  "org.example.Boo",
+  (GDBusMethodInfo **) NULL,
+  (GDBusSignalInfo **) NULL,
+  (GDBusPropertyInfo **) NULL,
+  NULL,
+};
+
+static const GDBusInterfaceVTable boo_vtable =
+{
+  NULL, /* _method_call */
+  NULL, /* _get_property */
+  NULL  /* _set_property */
+};
+
+static gboolean
+some_filter_func (GDBusConnection *connection,
+                  GDBusMessage    *message,
+                  gboolean         incoming,
+                  gpointer         user_data)
+{
+  return FALSE;
+}
+
+static void
+on_name_owner_changed (GDBusConnection *connection,
+                       const gchar     *sender_name,
+                       const gchar     *object_path,
+                       const gchar     *interface_name,
+                       const gchar     *signal_name,
+                       GVariant        *parameters,
+                       gpointer         user_data)
+{
+}
+
+static void
+a_gdestroynotify_that_sets_a_gboolean_to_true_and_quits_loop (gpointer user_data)
+{
+  gboolean *val = user_data;
+  *val = TRUE;
+
+  g_main_loop_quit (loop);
+}
+
 static void
 test_connection_life_cycle (void)
 {
@@ -43,6 +98,12 @@ test_connection_life_cycle (void)
   GDBusConnection *c;
   GDBusConnection *c2;
   GError *error;
+  gboolean on_signal_registration_freed_called;
+  gboolean on_filter_freed_called;
+  gboolean on_register_object_freed_called;
+  gboolean quit_mainloop_fired;
+  guint quit_mainloop_id;
+  guint registration_id;
 
   error = NULL;
 
@@ -100,6 +161,66 @@ test_connection_life_cycle (void)
   g_object_unref (c2);
 
   /*
+   * Check that the finalization code works
+   *
+   * (and that the GDestroyNotify for filters and objects and signal
+   * registrations are run as expected)
+   */
+  error = NULL;
+  c2 = _g_bus_get_priv (G_BUS_TYPE_SESSION, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (c2 != NULL);
+  /* signal registration */
+  on_signal_registration_freed_called = FALSE;
+  g_dbus_connection_signal_subscribe (c2,
+                                      "org.freedesktop.DBus", /* bus name */
+                                      "org.freedesktop.DBus", /* interface */
+                                      "NameOwnerChanged",     /* member */
+                                      "/org/freesktop/DBus",  /* path */
+                                      NULL,                   /* arg0 */
+                                      G_DBUS_SIGNAL_FLAGS_NONE,
+                                      on_name_owner_changed,
+                                      &on_signal_registration_freed_called,
+                                      a_gdestroynotify_that_sets_a_gboolean_to_true_and_quits_loop);
+  /* filter func */
+  on_filter_freed_called = FALSE;
+  g_dbus_connection_add_filter (c2,
+                                some_filter_func,
+                                &on_filter_freed_called,
+                                a_gdestroynotify_that_sets_a_gboolean_to_true_and_quits_loop);
+  /* object registration */
+  on_register_object_freed_called = FALSE;
+  error = NULL;
+  registration_id = g_dbus_connection_register_object (c2,
+                                                       "/foo",
+                                                       (GDBusInterfaceInfo *) &boo_interface_info,
+                                                       &boo_vtable,
+                                                       &on_register_object_freed_called,
+                                                       a_gdestroynotify_that_sets_a_gboolean_to_true_and_quits_loop,
+                                                       &error);
+  g_assert_no_error (error);
+  g_assert (registration_id > 0);
+  /* ok, finalize the connection and check that all the GDestroyNotify functions are invoked as expected */
+  g_object_unref (c2);
+  quit_mainloop_fired = FALSE;
+  quit_mainloop_id = g_timeout_add (1000, test_connection_quit_mainloop, &quit_mainloop_fired);
+  while (TRUE)
+    {
+      if (on_signal_registration_freed_called &&
+          on_filter_freed_called &&
+          on_register_object_freed_called)
+        break;
+      if (quit_mainloop_fired)
+        break;
+      g_main_loop_run (loop);
+    }
+  g_source_remove (quit_mainloop_id);
+  g_assert (on_signal_registration_freed_called);
+  g_assert (on_filter_freed_called);
+  g_assert (on_register_object_freed_called);
+  g_assert (!quit_mainloop_fired);
+
+  /*
    *  Check for correct behavior when the bus goes away
    *
    */
@@ -353,15 +474,6 @@ test_connection_signal_handler (GDBusConnection  *connection,
   g_main_loop_quit (loop);
 }
 
-static gboolean
-test_connection_signal_quit_mainloop (gpointer user_data)
-{
-  gboolean *quit_mainloop_fired = user_data;
-  *quit_mainloop_fired = TRUE;
-  g_main_loop_quit (loop);
-  return TRUE;
-}
-
 static void
 test_connection_signals (void)
 {
@@ -539,7 +651,7 @@ test_connection_signals (void)
   gboolean quit_mainloop_fired;
   guint quit_mainloop_id;
   quit_mainloop_fired = FALSE;
-  quit_mainloop_id = g_timeout_add (5000, test_connection_signal_quit_mainloop, &quit_mainloop_fired);
+  quit_mainloop_id = g_timeout_add (5000, test_connection_quit_mainloop, &quit_mainloop_fired);
   while (count_name_owner_changed < 2 && !quit_mainloop_fired)
     g_main_loop_run (loop);
   g_source_remove (quit_mainloop_id);