From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Philip Withnall 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 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) ()); }