[glib] gdbus: fix race condition in connection filter freeing

[Dan Winship <danw src gnome org>]

commit 7da3922d051907ccd9b32de140bab217c7665c02
Author: Dan Winship <danw gnome org>
Date:   Fri Aug 21 17:39:44 2015 -0400

    gdbus: fix race condition in connection filter freeing
    
    If you called g_dbus_connection_remove_filter() on a filter while it
    was running (or about to be run) in another thread, its GDestroyNotify
    would be run immediately, potentially causing the filter thread to
    crash.
    
    Fix this by refcounting the filters, and using the existing mechanism
    for running a GDestroyNotify in another thread in the case where the
    the gdbus thread is the one that frees it.
    
    Also, add a bit of documentation explaining this (and add a related
    clarification to g_dbus_connection_signal_subscribe()).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=704568

 gio/gdbusconnection.c |  136 ++++++++++++++++++++++++++++++++++---------------
 1 files changed, 95 insertions(+), 41 deletions(-)
---
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 4a8d9a5..d3f3f50 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -2163,18 +2163,53 @@ g_dbus_connection_send_message_with_reply_sync (GDBusConnection        *connecti
 
 typedef struct
 {
-  GDBusMessageFilterFunction func;
-  gpointer user_data;
-} FilterCallback;
-
-typedef struct
-{
   guint                       id;
+  guint                       ref_count;
   GDBusMessageFilterFunction  filter_function;
   gpointer                    user_data;
   GDestroyNotify              user_data_free_func;
+  GMainContext               *context;
 } FilterData;
 
+/* requires CONNECTION_LOCK */
+static FilterData **
+copy_filter_list (GPtrArray *filters)
+{
+  FilterData **copy;
+  guint n;
+
+  copy = g_new (FilterData *, filters->len + 1);
+  for (n = 0; n < filters->len; n++)
+    {
+      copy[n] = filters->pdata[n];
+      copy[n]->ref_count++;
+    }
+  copy[n] = NULL;
+
+  return copy;
+}
+
+/* requires CONNECTION_LOCK */
+static void
+free_filter_list (FilterData **filters)
+{
+  guint n;
+
+  for (n = 0; filters[n]; n++)
+    {
+      filters[n]->ref_count--;
+      if (filters[n]->ref_count == 0)
+        {
+          call_destroy_notify (filters[n]->context,
+                               filters[n]->user_data_free_func,
+                               filters[n]->user_data);
+          g_main_context_unref (filters[n]->context);
+          g_free (filters[n]);
+        }
+    }
+  g_free (filters);
+}
+
 /* Called in GDBusWorker's thread - we must not block - with no lock held */
 static void
 on_worker_message_received (GDBusWorker  *worker,
@@ -2182,8 +2217,7 @@ on_worker_message_received (GDBusWorker  *worker,
                             gpointer      user_data)
 {
   GDBusConnection *connection;
-  FilterCallback *filters;
-  guint num_filters;
+  FilterData **filters;
   guint n;
   gboolean alive;
 
@@ -2207,28 +2241,25 @@ on_worker_message_received (GDBusWorker  *worker,
 
   /* First collect the set of callback functions */
   CONNECTION_LOCK (connection);
-  num_filters = connection->filters->len;
-  filters = g_new0 (FilterCallback, num_filters);
-  for (n = 0; n < num_filters; n++)
-    {
-      FilterData *data = connection->filters->pdata[n];
-      filters[n].func = data->filter_function;
-      filters[n].user_data = data->user_data;
-    }
+  filters = copy_filter_list (connection->filters);
   CONNECTION_UNLOCK (connection);
 
   /* then call the filters in order (without holding the lock) */
-  for (n = 0; n < num_filters; n++)
+  for (n = 0; filters[n]; n++)
     {
-      message = filters[n].func (connection,
-                                 message,
-                                 TRUE,
-                                 filters[n].user_data);
+      message = filters[n]->filter_function (connection,
+                                             message,
+                                             TRUE,
+                                             filters[n]->user_data);
       if (message == NULL)
         break;
       g_dbus_message_lock (message);
     }
 
+  CONNECTION_LOCK (connection);
+  free_filter_list (filters);
+  CONNECTION_UNLOCK (connection);
+
   /* Standard dispatch unless the filter ate the message - no need to
    * do anything if the message was altered
    */
@@ -2274,7 +2305,6 @@ on_worker_message_received (GDBusWorker  *worker,
   if (message != NULL)
     g_object_unref (message);
   g_object_unref (connection);
-  g_free (filters);
 }
 
 /* Called in GDBusWorker's thread, lock is not held */
@@ -2284,8 +2314,7 @@ on_worker_message_about_to_be_sent (GDBusWorker  *worker,
                                     gpointer      user_data)
 {
   GDBusConnection *connection;
-  FilterCallback *filters;
-  guint num_filters;
+  FilterData **filters;
   guint n;
   gboolean alive;
 
@@ -2304,30 +2333,26 @@ on_worker_message_about_to_be_sent (GDBusWorker  *worker,
 
   /* First collect the set of callback functions */
   CONNECTION_LOCK (connection);
-  num_filters = connection->filters->len;
-  filters = g_new0 (FilterCallback, num_filters);
-  for (n = 0; n < num_filters; n++)
-    {
-      FilterData *data = connection->filters->pdata[n];
-      filters[n].func = data->filter_function;
-      filters[n].user_data = data->user_data;
-    }
+  filters = copy_filter_list (connection->filters);
   CONNECTION_UNLOCK (connection);
 
   /* then call the filters in order (without holding the lock) */
-  for (n = 0; n < num_filters; n++)
+  for (n = 0; filters[n]; n++)
     {
       g_dbus_message_lock (message);
-      message = filters[n].func (connection,
-                                 message,
-                                 FALSE,
-                                 filters[n].user_data);
+      message = filters[n]->filter_function (connection,
+                                             message,
+                                             FALSE,
+                                             filters[n]->user_data);
       if (message == NULL)
         break;
     }
 
+  CONNECTION_LOCK (connection);
+  free_filter_list (filters);
+  CONNECTION_UNLOCK (connection);
+
   g_object_unref (connection);
-  g_free (filters);
 
   return message;
 }
@@ -3059,6 +3084,13 @@ static guint _global_filter_id = 1;
  * message. Similary, if a filter consumes an outgoing message, the
  * message will not be sent to the other peer.
  *
+ * If @user_data_free_func is non-%NULL, it will be called (in the
+ * thread-default main context of the thread you are calling this
+ * method from) at some point after @user_data is no longer
+ * needed. (It is not guaranteed to be called synchronously when the
+ * filter is removed, and may be called after @connection has been
+ * destroyed.)
+ *
  * Returns: a filter identifier that can be used with
  *     g_dbus_connection_remove_filter()
  *
@@ -3079,9 +3111,11 @@ g_dbus_connection_add_filter (GDBusConnection            *connection,
   CONNECTION_LOCK (connection);
   data = g_new0 (FilterData, 1);
   data->id = _global_filter_id++; /* TODO: overflow etc. */
+  data->ref_count = 1;
   data->filter_function = filter_function;
   data->user_data = user_data;
   data->user_data_free_func = user_data_free_func;
+  data->context = g_main_context_ref_thread_default ();
   g_ptr_array_add (connection->filters, data);
   CONNECTION_UNLOCK (connection);
 
@@ -3096,8 +3130,11 @@ purge_all_filters (GDBusConnection *connection)
   for (n = 0; n < connection->filters->len; n++)
     {
       FilterData *data = connection->filters->pdata[n];
-      if (data->user_data_free_func != NULL)
-        data->user_data_free_func (data->user_data);
+
+      call_destroy_notify (data->context,
+                           data->user_data_free_func,
+                           data->user_data);
+      g_main_context_unref (data->context);
       g_free (data);
     }
 }
@@ -3109,6 +3146,13 @@ purge_all_filters (GDBusConnection *connection)
  *
  * Removes a filter.
  *
+ * Note that since filters run in a different thread, there is a race
+ * condition where it is possible that the filter will be running even
+ * after calling g_dbus_connection_remove_filter(), so you cannot just
+ * free data that the filter might be using. Instead, you should pass
+ * a #GDestroyNotify to g_dbus_connection_add_filter(), which will be
+ * called when it is guaranteed that the data is no longer needed.
+ *
  * Since: 2.26
  */
 void
@@ -3129,7 +3173,9 @@ g_dbus_connection_remove_filter (GDBusConnection *connection,
       if (data->id == filter_id)
         {
           g_ptr_array_remove_index (connection->filters, n);
-          to_destroy = data;
+          data->ref_count--;
+          if (data->ref_count == 0)
+            to_destroy = data;
           break;
         }
     }
@@ -3140,6 +3186,7 @@ g_dbus_connection_remove_filter (GDBusConnection *connection,
     {
       if (to_destroy->user_data_free_func != NULL)
         to_destroy->user_data_free_func (to_destroy->user_data);
+      g_main_context_unref (to_destroy->context);
       g_free (to_destroy);
     }
   else
@@ -3346,6 +3393,13 @@ is_signal_data_for_name_lost_or_acquired (SignalData *signal_data)
  * interpreted as part of a namespace or path.  The first argument
  * of a signal is matched against that part as specified by D-Bus.
  *
+ * If @user_data_free_func is non-%NULL, it will be called (in the
+ * thread-default main context of the thread you are calling this
+ * method from) at some point after @user_data is no longer
+ * needed. (It is not guaranteed to be called synchronously when the
+ * signal is unsubscribed from, and may be called after @connection
+ * has been destroyed.)
+ *
  * Returns: a subscription identifier that can be used with g_dbus_connection_signal_unsubscribe()
  *
  * Since: 2.26