1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
|
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
|
