From 82c764ce2e42f0d1032627dabcbd742d5f2bd8fa Mon Sep 17 00:00:00 2001
From: Philip Withnall <philip@tecnocode.co.uk>
Date: Mon, 11 Sep 2023 16:02:15 +0100
Subject: [PATCH] gthreadedresolver: Fix race between source callbacks and
 finalize
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

I had thought that because `g_source_destroy()` was called for the two
sources (cancel and timeout) in the `GTask` finalize function for a
threaded resolver operation, that it would be fine to use a plain
pointer in the source callbacks to point to the `GTask`.

That turns out to not be true: because the source callbacks are executed
in the GLib worker thread, and the `GTask` can be finalized in another
thread, it’s possible for a source callback (e.g. `cancelled_cb()`) to
be scheduled in the worker thread, then for the `GTask` to be finalized,
and then the source callback to continue execution and find itself
doing a use-after-free.

Fix that by using a weak ref to the `GTask` in the source callbacks,
rather than a plain pointer.

Signed-off-by: Philip Withnall <philip@tecnocode.co.uk>

Fixes: #3105
---
 gio/gthreadedresolver.c | 43 +++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/gio/gthreadedresolver.c b/gio/gthreadedresolver.c
index 2d94531bfd..c7a567549f 100644
--- a/gio/gthreadedresolver.c
+++ b/gio/gthreadedresolver.c
@@ -1422,10 +1422,17 @@ lookup_records_finish (GResolver     *resolver,
 static gboolean
 timeout_cb (gpointer user_data)
 {
-  GTask *task = G_TASK (user_data);
-  LookupData *data = g_task_get_task_data (task);
+  GWeakRef *weak_task = user_data;
+  GTask *task = NULL;  /* (owned) */
+  LookupData *data;
   gboolean should_return;
 
+  task = g_weak_ref_get (weak_task);
+  if (task == NULL)
+    return G_SOURCE_REMOVE;
+
+  data = g_task_get_task_data (task);
+
   g_mutex_lock (&data->lock);
 
   should_return = g_atomic_int_compare_and_exchange (&data->will_return, NOT_YET, TIMED_OUT);
@@ -1443,6 +1450,8 @@ timeout_cb (gpointer user_data)
   g_cond_broadcast (&data->cond);
   g_mutex_unlock (&data->lock);
 
+  g_object_unref (task);
+
   return G_SOURCE_REMOVE;
 }
 
@@ -1452,10 +1461,17 @@ static gboolean
 cancelled_cb (GCancellable *cancellable,
               gpointer      user_data)
 {
-  GTask *task = G_TASK (user_data);
-  LookupData *data = g_task_get_task_data (task);
+  GWeakRef *weak_task = user_data;
+  GTask *task = NULL;  /* (owned) */
+  LookupData *data;
   gboolean should_return;
 
+  task = g_weak_ref_get (weak_task);
+  if (task == NULL)
+    return G_SOURCE_REMOVE;
+
+  data = g_task_get_task_data (task);
+
   g_mutex_lock (&data->lock);
 
   g_assert (g_cancellable_is_cancelled (cancellable));
@@ -1473,9 +1489,18 @@ cancelled_cb (GCancellable *cancellable,
   g_cond_broadcast (&data->cond);
   g_mutex_unlock (&data->lock);
 
+  g_object_unref (task);
+
   return G_SOURCE_REMOVE;
 }
 
+static void
+weak_ref_clear_and_free (GWeakRef *weak_ref)
+{
+  g_weak_ref_clear (weak_ref);
+  g_free (weak_ref);
+}
+
 static void
 run_task_in_thread_pool_async (GThreadedResolver *self,
                                GTask             *task)
@@ -1490,17 +1515,23 @@ run_task_in_thread_pool_async (GThreadedResolver *self,
 
   if (timeout_ms != 0)
     {
+      GWeakRef *weak_task = g_new0 (GWeakRef, 1);
+      g_weak_ref_set (weak_task, task);
+
       data->timeout_source = g_timeout_source_new (timeout_ms);
       g_source_set_static_name (data->timeout_source, "[gio] threaded resolver timeout");
-      g_source_set_callback (data->timeout_source, G_SOURCE_FUNC (timeout_cb), task, NULL);
+      g_source_set_callback (data->timeout_source, G_SOURCE_FUNC (timeout_cb), g_steal_pointer (&weak_task), (GDestroyNotify) weak_ref_clear_and_free);
       g_source_attach (data->timeout_source, GLIB_PRIVATE_CALL (g_get_worker_context) ());
     }
 
   if (cancellable != NULL)
     {
+      GWeakRef *weak_task = g_new0 (GWeakRef, 1);
+      g_weak_ref_set (weak_task, task);
+
       data->cancellable_source = g_cancellable_source_new (cancellable);
       g_source_set_static_name (data->cancellable_source, "[gio] threaded resolver cancellable");
-      g_source_set_callback (data->cancellable_source, G_SOURCE_FUNC (cancelled_cb), task, NULL);
+      g_source_set_callback (data->cancellable_source, G_SOURCE_FUNC (cancelled_cb), g_steal_pointer (&weak_task), (GDestroyNotify) weak_ref_clear_and_free);
       g_source_attach (data->cancellable_source, GLIB_PRIVATE_CALL (g_get_worker_context) ());
     }
 
-- 
GitLab