159 lines
5.3 KiB
Diff
159 lines
5.3 KiB
Diff
From 0000000000000000000000000000000000000000 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 2d94531bfda3..c7a567549f28 100644
|
||
--- a/gio/gthreadedresolver.c
|
||
+++ b/gio/gthreadedresolver.c
|
||
@@ -1422,85 +1422,116 @@ 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);
|
||
g_clear_pointer (&data->timeout_source, g_source_unref);
|
||
|
||
g_mutex_unlock (&data->lock);
|
||
|
||
if (should_return)
|
||
g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT,
|
||
_("Socket I/O timed out"));
|
||
|
||
/* Signal completion of the task. */
|
||
g_mutex_lock (&data->lock);
|
||
data->has_returned = TRUE;
|
||
g_cond_broadcast (&data->cond);
|
||
g_mutex_unlock (&data->lock);
|
||
|
||
+ g_object_unref (task);
|
||
+
|
||
return G_SOURCE_REMOVE;
|
||
}
|
||
|
||
/* Will be called in the GLib worker thread, so must lock all accesses to shared
|
||
* data. */
|
||
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));
|
||
should_return = g_atomic_int_compare_and_exchange (&data->will_return, NOT_YET, CANCELLED);
|
||
g_clear_pointer (&data->cancellable_source, g_source_unref);
|
||
|
||
g_mutex_unlock (&data->lock);
|
||
|
||
if (should_return)
|
||
g_task_return_error_if_cancelled (task);
|
||
|
||
/* Signal completion of the task. */
|
||
g_mutex_lock (&data->lock);
|
||
data->has_returned = TRUE;
|
||
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)
|
||
{
|
||
LookupData *data = g_task_get_task_data (task);
|
||
guint timeout_ms = g_resolver_get_timeout (G_RESOLVER (self));
|
||
GCancellable *cancellable = g_task_get_cancellable (task);
|
||
|
||
g_mutex_lock (&data->lock);
|
||
|
||
g_thread_pool_push (self->thread_pool, g_object_ref (task), NULL);
|
||
|
||
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) ());
|
||
}
|
||
|