From 7e8d4145af745e6ac51337ffcc65872791e7299f Mon Sep 17 00:00:00 2001
From: Dan Winship <danw@gnome.org>
Date: Mon, 6 Apr 2015 10:09:04 -0400
Subject: [PATCH] gdbus: fix deadlock on message cancel/timeout

The gdbus GTask port introduced a deadlock because some code had been
using g_simple_async_result_complete_in_idle() to ensure that the
callback didn't run until after a mutex was unlocked, but in the gtask
version, the callback was being run immediately. Fix it to drop the
mutex before calling g_task_return*(). Also, tweak
tests/gdbus-connection to test this.

https://bugzilla.gnome.org/show_bug.cgi?id=747349
---
 gio/gdbusconnection.c        | 62 +++++++++++++++++-------------------
 gio/tests/gdbus-connection.c | 15 +++++++++
 2 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 50f792095..9ebf6d299 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -1803,7 +1803,7 @@ send_message_with_reply_cleanup (GTask *task, gboolean remove)
 
 /* ---------------------------------------------------------------------------------------------------- */
 
-/* Can be called from any thread with lock held */
+/* Called from GDBus worker thread with lock held */
 static void
 send_message_data_deliver_reply_unlocked (GTask           *task,
                                           GDBusMessage    *reply)
@@ -1821,6 +1821,31 @@ send_message_data_deliver_reply_unlocked (GTask           *task,
   ;
 }
 
+/* Called from a user thread, lock is not held */
+static void
+send_message_data_deliver_error (GTask      *task,
+                                 GQuark      domain,
+                                 gint        code,
+                                 const char *message)
+{
+  GDBusConnection *connection = g_task_get_source_object (task);
+  SendMessageData *data = g_task_get_task_data (task);
+
+  CONNECTION_LOCK (connection);
+  if (data->delivered)
+    {
+      CONNECTION_UNLOCK (connection);
+      return;
+    }
+
+  g_object_ref (task);
+  send_message_with_reply_cleanup (task, TRUE);
+  CONNECTION_UNLOCK (connection);
+
+  g_task_return_new_error (task, domain, code, "%s", message);
+  g_object_unref (task);
+}
+
 /* ---------------------------------------------------------------------------------------------------- */
 
 /* Called from a user thread, lock is not held */
@@ -1828,22 +1853,9 @@ static gboolean
 send_message_with_reply_cancelled_idle_cb (gpointer user_data)
 {
   GTask *task = user_data;
-  GDBusConnection *connection = g_task_get_source_object (task);
-  SendMessageData *data = g_task_get_task_data (task);
 
-  CONNECTION_LOCK (connection);
-  if (data->delivered)
-    goto out;
-
-  g_task_return_new_error (task,
-                           G_IO_ERROR,
-                           G_IO_ERROR_CANCELLED,
-                           _("Operation was cancelled"));
-
-  send_message_with_reply_cleanup (task, TRUE);
-
- out:
-  CONNECTION_UNLOCK (connection);
+  send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_CANCELLED,
+                                   _("Operation was cancelled"));
   return FALSE;
 }
 
@@ -1871,23 +1883,9 @@ static gboolean
 send_message_with_reply_timeout_cb (gpointer user_data)
 {
   GTask *task = user_data;
-  GDBusConnection *connection = g_task_get_source_object (task);
-  SendMessageData *data = g_task_get_task_data (task);
-
-  CONNECTION_LOCK (connection);
-  if (data->delivered)
-    goto out;
-
-  g_task_return_new_error (task,
-                           G_IO_ERROR,
-                           G_IO_ERROR_TIMED_OUT,
-                           _("Timeout was reached"));
-
-  send_message_with_reply_cleanup (task, TRUE);
-
- out:
-  CONNECTION_UNLOCK (connection);
 
+  send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT,
+                                   _("Timeout was reached"));
   return FALSE;
 }
 
diff --git a/gio/tests/gdbus-connection.c b/gio/tests/gdbus-connection.c
index 8d0b0dfc3..80cec2246 100644
--- a/gio/tests/gdbus-connection.c
+++ b/gio/tests/gdbus-connection.c
@@ -291,6 +291,9 @@ msg_cb_expect_error_disconnected (GDBusConnection *connection,
   GError *error;
   GVariant *result;
 
+  /* Make sure gdbusconnection isn't holding @connection's lock. (#747349) */
+  g_dbus_connection_get_last_serial (connection);
+
   error = NULL;
   result = g_dbus_connection_call_finish (connection,
                                           res,
@@ -311,6 +314,9 @@ msg_cb_expect_error_unknown_method (GDBusConnection *connection,
   GError *error;
   GVariant *result;
 
+  /* Make sure gdbusconnection isn't holding @connection's lock. (#747349) */
+  g_dbus_connection_get_last_serial (connection);
+
   error = NULL;
   result = g_dbus_connection_call_finish (connection,
                                           res,
@@ -331,6 +337,9 @@ msg_cb_expect_success (GDBusConnection *connection,
   GError *error;
   GVariant *result;
 
+  /* Make sure gdbusconnection isn't holding @connection's lock. (#747349) */
+  g_dbus_connection_get_last_serial (connection);
+
   error = NULL;
   result = g_dbus_connection_call_finish (connection,
                                           res,
@@ -350,6 +359,9 @@ msg_cb_expect_error_cancelled (GDBusConnection *connection,
   GError *error;
   GVariant *result;
 
+  /* Make sure gdbusconnection isn't holding @connection's lock. (#747349) */
+  g_dbus_connection_get_last_serial (connection);
+
   error = NULL;
   result = g_dbus_connection_call_finish (connection,
                                           res,
@@ -370,6 +382,9 @@ msg_cb_expect_error_cancelled_2 (GDBusConnection *connection,
   GError *error;
   GVariant *result;
 
+  /* Make sure gdbusconnection isn't holding @connection's lock. (#747349) */
+  g_dbus_connection_get_last_serial (connection);
+
   error = NULL;
   result = g_dbus_connection_call_finish (connection,
                                           res,