Merge branch 'backport-3291-dbus-connection-refcounting-glib-2-76' into 'glib-2-76'

Backport !3291 “gdbusconnection: Fix double unref on timeout/cancel sending a message” to glib-2-76

See merge request GNOME/glib!3427

2 files changed, 72 insertions, 56 deletions

diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index ccca4e98c..65ece54b2 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -409,7 +409,7 @@ struct _GDBusConnection
   GDBusConnectionFlags flags;
 
   /* Map used for managing method replies, protected by @lock */
-  GHashTable *map_method_serial_to_task;  /* guint32 -> GTask* */
+  GHashTable *map_method_serial_to_task;  /* guint32 -> owned GTask* */
 
   /* Maps used for managing signal subscription, protected by @lock */
   GHashTable *map_rule_to_signal_data;                      /* match rule (gchar*)    -> SignalData */
@@ -1061,7 +1061,7 @@ g_dbus_connection_init (GDBusConnection *connection)
   g_mutex_init (&connection->lock);
   g_mutex_init (&connection->init_lock);
 
-  connection->map_method_serial_to_task = g_hash_table_new (g_direct_hash, g_direct_equal);
+  connection->map_method_serial_to_task = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref);
 
   connection->map_rule_to_signal_data = g_hash_table_new (g_str_hash,
                                                           g_str_equal);
@@ -1752,7 +1752,7 @@ typedef struct
   gulong cancellable_handler_id;
   GSource *cancelled_idle_source;  /* (owned) (nullable) */
 
-  GSource *timeout_source;
+  GSource *timeout_source;  /* (owned) (nullable) */
 
   gboolean delivered;
 } SendMessageData;
@@ -1761,6 +1761,7 @@ typedef struct
 static void
 send_message_data_free (SendMessageData *data)
 {
+  /* These should already have been cleared by send_message_with_reply_cleanup(). */
   g_assert (data->timeout_source == NULL);
   g_assert (data->cancellable_handler_id == 0);
 
@@ -1769,7 +1770,7 @@ send_message_data_free (SendMessageData *data)
 
 /* ---------------------------------------------------------------------------------------------------- */
 
-/* can be called from any thread with lock held; @task is (transfer full) */
+/* can be called from any thread with lock held; @task is (transfer none) */
 static void
 send_message_with_reply_cleanup (GTask *task, gboolean remove)
 {
@@ -1785,7 +1786,7 @@ send_message_with_reply_cleanup (GTask *task, gboolean remove)
   if (data->timeout_source != NULL)
     {
       g_source_destroy (data->timeout_source);
-      data->timeout_source = NULL;
+      g_clear_pointer (&data->timeout_source, g_source_unref);
     }
   if (data->cancellable_handler_id > 0)
     {
@@ -1804,13 +1805,11 @@ send_message_with_reply_cleanup (GTask *task, gboolean remove)
                                               GUINT_TO_POINTER (data->serial));
       g_warn_if_fail (removed);
     }
-
-  g_object_unref (task);
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
 
-/* Called from GDBus worker thread with lock held; @task is (transfer full). */
+/* Called from GDBus worker thread with lock held; @task is (transfer none). */
 static void
 send_message_data_deliver_reply_unlocked (GTask           *task,
                                           GDBusMessage    *reply)
@@ -1828,7 +1827,7 @@ send_message_data_deliver_reply_unlocked (GTask           *task,
   ;
 }
 
-/* Called from a user thread, lock is not held */
+/* Called from a user thread, lock is not held; @task is (transfer none) */
 static void
 send_message_data_deliver_error (GTask      *task,
                                  GQuark      domain,
@@ -1845,7 +1844,10 @@ send_message_data_deliver_error (GTask      *task,
       return;
     }
 
+  /* Hold a ref on @task as send_message_with_reply_cleanup() will remove it
+   * from the task map and could end up dropping the last reference */
   g_object_ref (task);
+
   send_message_with_reply_cleanup (task, TRUE);
   CONNECTION_UNLOCK (connection);
 
@@ -1855,7 +1857,7 @@ send_message_data_deliver_error (GTask      *task,
 
 /* ---------------------------------------------------------------------------------------------------- */
 
-/* Called from a user thread, lock is not held; @task is (transfer full) */
+/* Called from a user thread, lock is not held; @task is (transfer none) */
 static gboolean
 send_message_with_reply_cancelled_idle_cb (gpointer user_data)
 {
@@ -1887,7 +1889,7 @@ send_message_with_reply_cancelled_cb (GCancellable *cancellable,
 
 /* ---------------------------------------------------------------------------------------------------- */
 
-/* Called from a user thread, lock is not held; @task is (transfer full) */
+/* Called from a user thread, lock is not held; @task is (transfer none) */
 static gboolean
 send_message_with_reply_timeout_cb (gpointer user_data)
 {
@@ -1895,7 +1897,7 @@ send_message_with_reply_timeout_cb (gpointer user_data)
 
   send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT,
                                    _("Timeout was reached"));
-  return FALSE;
+  return G_SOURCE_REMOVE;
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
@@ -1956,7 +1958,6 @@ g_dbus_connection_send_message_with_reply_unlocked (GDBusConnection     *connect
       g_source_set_static_name (data->timeout_source, "[gio] send_message_with_reply_unlocked");
       g_task_attach_source (task, data->timeout_source,
                             (GSourceFunc) send_message_with_reply_timeout_cb);
-      g_source_unref (data->timeout_source);
     }
 
   g_hash_table_insert (connection->map_method_serial_to_task,
@@ -2397,7 +2398,8 @@ on_worker_message_about_to_be_sent (GDBusWorker  *worker,
   return message;
 }
 
-/* called with connection lock held, in GDBusWorker thread */
+/* called with connection lock held, in GDBusWorker thread
+ * @key, @value and @user_data are (transfer none) */
 static gboolean
 cancel_method_on_close (gpointer key, gpointer value, gpointer user_data)
 {
@@ -3749,7 +3751,7 @@ g_dbus_connection_signal_unsubscribe (GDBusConnection *connection,
 typedef struct
 {
   SignalSubscriber    *subscriber;  /* (owned) */
-  GDBusMessage        *message;
+  GDBusMessage        *message;  /* (owned) */
   GDBusConnection     *connection;
   const gchar         *sender;  /* (nullable) for peer-to-peer connections */
   const gchar         *path;
@@ -3813,7 +3815,7 @@ emit_signal_instance_in_idle_cb (gpointer data)
 static void
 signal_instance_free (SignalInstance *signal_instance)
 {
-  g_object_unref (signal_instance->message);
+  g_clear_object (&signal_instance->message);
   g_object_unref (signal_instance->connection);
   signal_subscriber_unref (signal_instance->subscriber);
   g_free (signal_instance);
@@ -4225,7 +4227,7 @@ has_object_been_unregistered (GDBusConnection    *connection,
 typedef struct
 {
   GDBusConnection *connection;
-  GDBusMessage *message;
+  GDBusMessage *message;  /* (owned) */
   gpointer user_data;
   const gchar *property_name;
   const GDBusInterfaceVTable *vtable;
@@ -4239,7 +4241,7 @@ static void
 property_data_free (PropertyData *data)
 {
   g_object_unref (data->connection);
-  g_object_unref (data->message);
+  g_clear_object (&data->message);
   g_free (data);
 }
 
@@ -4581,7 +4583,7 @@ handle_getset_property (GDBusConnection *connection,
 typedef struct
 {
   GDBusConnection *connection;
-  GDBusMessage *message;
+  GDBusMessage *message;  /* (owned) */
   gpointer user_data;
   const GDBusInterfaceVTable *vtable;
   GDBusInterfaceInfo *interface_info;
@@ -4590,10 +4592,10 @@ typedef struct
 } PropertyGetAllData;
 
 static void
-property_get_all_data_free (PropertyData *data)
+property_get_all_data_free (PropertyGetAllData *data)
 {
   g_object_unref (data->connection);
-  g_object_unref (data->message);
+  g_clear_object (&data->message);
   g_free (data);
 }
 
@@ -6821,7 +6823,7 @@ typedef struct
 static void
 subtree_deferred_data_free (SubtreeDeferredData *data)
 {
-  g_object_unref (data->message);
+  g_clear_object (&data->message);
   exported_subtree_unref (data->es);
   g_free (data);
 }
diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c
index 762afcee4..2c9238c63 100644
--- a/gio/gdbusprivate.c
+++ b/gio/gdbusprivate.c
@@ -889,21 +889,25 @@ _g_dbus_worker_do_initial_read (gpointer data)
 struct _MessageToWriteData
 {
   GDBusWorker  *worker;
-  GDBusMessage *message;
+  GDBusMessage *message;  /* (owned) */
   gchar        *blob;
   gsize         blob_size;
 
   gsize         total_written;
-  GTask        *task;
+  GTask        *task;  /* (owned) and (nullable) before writing starts and after g_task_return_*() is called */
 };
 
 static void
 message_to_write_data_free (MessageToWriteData *data)
 {
   _g_dbus_worker_unref (data->worker);
-  if (data->message)
-    g_object_unref (data->message);
+  g_clear_object (&data->message);
   g_free (data->blob);
+
+  /* The task must either not have been created, or have been created, returned
+   * and finalised by now. */
+  g_assert (data->task == NULL);
+
   g_slice_free (MessageToWriteData, data);
 }
 
@@ -915,21 +919,22 @@ static void write_message_continue_writing (MessageToWriteData *data);
  *
  * write-lock is not held on entry
  * output_pending is PENDING_WRITE on entry
+ * @user_data is (transfer full)
  */
 static void
 write_message_async_cb (GObject      *source_object,
                         GAsyncResult *res,
                         gpointer      user_data)
 {
-  MessageToWriteData *data = user_data;
-  GTask *task;
+  MessageToWriteData *data = g_steal_pointer (&user_data);
   gssize bytes_written;
   GError *error;
 
-  /* Note: we can't access data->task after calling g_task_return_* () because the
-   * callback can free @data and we're not completing in idle. So use a copy of the pointer.
-   */
-  task = data->task;
+  /* The ownership of @data is a bit odd in this function: it’s (transfer full)
+   * when the function is called, but the code paths which call g_task_return_*()
+   * on @data->task will indirectly cause it to be freed, because @data is
+   * always guaranteed to be the user_data in the #GTask. So that’s why it looks
+   * like @data is not always freed on every code path in this function. */
 
   error = NULL;
   bytes_written = g_output_stream_write_finish (G_OUTPUT_STREAM (source_object),
@@ -937,8 +942,9 @@ write_message_async_cb (GObject      *source_object,
                                                 &error);
   if (bytes_written == -1)
     {
+      GTask *task = g_steal_pointer (&data->task);
       g_task_return_error (task, error);
-      g_object_unref (task);
+      g_clear_object (&task);
       goto out;
     }
   g_assert (bytes_written > 0); /* zero is never returned */
@@ -949,12 +955,13 @@ write_message_async_cb (GObject      *source_object,
   g_assert (data->total_written <= data->blob_size);
   if (data->total_written == data->blob_size)
     {
+      GTask *task = g_steal_pointer (&data->task);
       g_task_return_boolean (task, TRUE);
-      g_object_unref (task);
+      g_clear_object (&task);
       goto out;
     }
 
-  write_message_continue_writing (data);
+  write_message_continue_writing (g_steal_pointer (&data));
 
  out:
   ;
@@ -971,9 +978,9 @@ on_socket_ready (GSocket      *socket,
                  GIOCondition  condition,
                  gpointer      user_data)
 {
-  MessageToWriteData *data = user_data;
-  write_message_continue_writing (data);
-  return FALSE; /* remove source */
+  MessageToWriteData *data = g_steal_pointer (&user_data);
+  write_message_continue_writing (g_steal_pointer (&data));
+  return G_SOURCE_REMOVE;
 }
 #endif
 
@@ -981,22 +988,21 @@ on_socket_ready (GSocket      *socket,
  *
  * write-lock is not held on entry
  * output_pending is PENDING_WRITE on entry
+ * @data is (transfer full)
  */
 static void
 write_message_continue_writing (MessageToWriteData *data)
 {
   GOutputStream *ostream;
 #ifdef G_OS_UNIX
-  GTask *task;
   GUnixFDList *fd_list;
 #endif
 
-#ifdef G_OS_UNIX
-  /* Note: we can't access data->task after calling g_task_return_* () because the
-   * callback can free @data and we're not completing in idle. So use a copy of the pointer.
-   */
-  task = data->task;
-#endif
+  /* The ownership of @data is a bit odd in this function: it’s (transfer full)
+   * when the function is called, but the code paths which call g_task_return_*()
+   * on @data->task will indirectly cause it to be freed, because @data is
+   * always guaranteed to be the user_data in the #GTask. So that’s why it looks
+   * like @data is not always freed on every code path in this function. */
 
   ostream = g_io_stream_get_output_stream (data->worker->stream);
 #ifdef G_OS_UNIX
@@ -1025,11 +1031,12 @@ write_message_continue_writing (MessageToWriteData *data)
         {
           if (!(data->worker->capabilities & G_DBUS_CAPABILITY_FLAGS_UNIX_FD_PASSING))
             {
+              GTask *task = g_steal_pointer (&data->task);
               g_task_return_new_error (task,
                                        G_IO_ERROR,
                                        G_IO_ERROR_FAILED,
                                        "Tried sending a file descriptor but remote peer does not support this capability");
-              g_object_unref (task);
+              g_clear_object (&task);
               goto out;
             }
           control_message = g_unix_fd_message_new_with_fd_list (fd_list);
@@ -1059,16 +1066,20 @@ write_message_continue_writing (MessageToWriteData *data)
                                                data->worker->cancellable);
               g_source_set_callback (source,
                                      (GSourceFunc) on_socket_ready,
-                                     data,
+                                     g_steal_pointer (&data),
                                      NULL); /* GDestroyNotify */
               g_source_attach (source, g_main_context_get_thread_default ());
               g_source_unref (source);
               g_error_free (error);
               goto out;
             }
-          g_task_return_error (task, error);
-          g_object_unref (task);
-          goto out;
+          else
+            {
+              GTask *task = g_steal_pointer (&data->task);
+              g_task_return_error (task, error);
+              g_clear_object (&task);
+              goto out;
+            }
         }
       g_assert (bytes_written > 0); /* zero is never returned */
 
@@ -1078,12 +1089,13 @@ write_message_continue_writing (MessageToWriteData *data)
       g_assert (data->total_written <= data->blob_size);
       if (data->total_written == data->blob_size)
         {
+          GTask *task = g_steal_pointer (&data->task);
           g_task_return_boolean (task, TRUE);
-          g_object_unref (task);
+          g_clear_object (&task);
           goto out;
         }
 
-      write_message_continue_writing (data);
+      write_message_continue_writing (g_steal_pointer (&data));
     }
 #endif
   else
@@ -1094,12 +1106,13 @@ write_message_continue_writing (MessageToWriteData *data)
           /* We were trying to write byte 0 of the message, which needs
            * the fd list to be attached to it, but this connection doesn't
            * support doing that. */
+          GTask *task = g_steal_pointer (&data->task);
           g_task_return_new_error (task,
                                    G_IO_ERROR,
                                    G_IO_ERROR_FAILED,
                                    "Tried sending a file descriptor on unsupported stream of type %s",
                                    g_type_name (G_TYPE_FROM_INSTANCE (ostream)));
-          g_object_unref (task);
+          g_clear_object (&task);
           goto out;
         }
 #endif
@@ -1110,7 +1123,7 @@ write_message_continue_writing (MessageToWriteData *data)
                                    G_PRIORITY_DEFAULT,
                                    data->worker->cancellable,
                                    write_message_async_cb,
-                                   data);
+                                   data);  /* steal @data */
     }
 #ifdef G_OS_UNIX
  out:
@@ -1133,7 +1146,7 @@ write_message_async (GDBusWorker         *worker,
   g_task_set_source_tag (data->task, write_message_async);
   g_task_set_name (data->task, "[gio] D-Bus write message");
   data->total_written = 0;
-  write_message_continue_writing (data);
+  write_message_continue_writing (g_steal_pointer (&data));
 }
 
 /* called in private thread shared by all GDBusConnection instances (with write-lock held) */
@@ -1322,6 +1335,7 @@ prepare_flush_unlocked (GDBusWorker *worker)
  *
  * write-lock is not held on entry
  * output_pending is PENDING_WRITE on entry
+ * @user_data is (transfer full)
  */
 static void
 write_message_cb (GObject       *source_object,
@@ -1540,7 +1554,7 @@ continue_writing (GDBusWorker *worker)
       write_message_async (worker,
                            data,
                            write_message_cb,
-                           data);
+                           data);  /* takes ownership of @data as user_data */
     }
 }