[glib] Bug 625584 – Crashes application on unref with signal subscription

[David Zeuthen <davidz src gnome org>]

commit d2d97a214d1d9e96f09955212e669c3c9447ff73
Author: David Zeuthen <davidz redhat com>
Date:   Fri Jul 30 16:01:03 2010 -0400

    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>

 gio/gdbusconnection.c        |   19 ++++--
 gio/tests/gdbus-connection.c |  132 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 134 insertions(+), 17 deletions(-)
---
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 116d547..47b347f 100644
--- 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
--- 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);