From d7c8bb0726908dba3d349bec5599b5a54f5861a1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <mail@3v1n0.net>
Date: Fri, 23 Aug 2024 16:37:48 -0400
Subject: [PATCH 1/2] gcancellable: Ignore cancelled callback on disposed
source
Prevent to access to a disposed gsource in the GCancellable cancelled
signal callback.
Due to the fact that the signal is called in the same thread in which
the cancellable get cancelled, we may receive the callback just after
the GSource has been disposed or during its disposal.
To prevent this, let's pass to the signal a pointer to the source itself
and nullify atomically the first time we're calling a callback or
disposing it.
In case the dispose function wins the race, then the callback will just
do nothing, while the disposal will continue as expected.
In case the callback wins the race, during the concurrent disposal we'll
just wait for the callback to be completed, before returning the disposal
itself that will likely lead to freeing the GSource.
Closes: #3448
---
gio/gcancellable.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/gio/gcancellable.c b/gio/gcancellable.c
index 27dd06370..892d1a9cb 100644
--- a/gio/gcancellable.c
+++ b/gio/gcancellable.c
@@ -640,6 +640,8 @@ g_cancellable_disconnect (GCancellable *cancellable,
typedef struct {
GSource source;
+ /* Atomic: */
+ GSource **self_ptr;
/* Atomic: */
GCancellable *cancellable;
gulong cancelled_handler;
@@ -662,10 +664,16 @@ static void
cancellable_source_cancelled (GCancellable *cancellable,
gpointer user_data)
{
- GSource *source = user_data;
- GCancellableSource *cancellable_source = (GCancellableSource *) source;
+ GSource *source = g_atomic_pointer_exchange ((GSource **) user_data, NULL);
+ GCancellableSource *cancellable_source;
gboolean callback_was_not_called G_GNUC_UNUSED;
+ /* The source is being disposed, so don't bother marking it as ready */
+ if (source == NULL)
+ return;
+
+ cancellable_source = (GCancellableSource *) source;
+
g_source_ref (source);
g_source_set_ready_time (source, 0);
@@ -688,7 +696,10 @@ cancellable_source_prepare (GSource *source,
cancellable = g_atomic_pointer_get (&cancellable_source->cancellable);
if (cancellable && !g_atomic_int_get (&cancellable->priv->cancelled_running))
- g_atomic_int_set (&cancellable_source->cancelled_callback_called, FALSE);
+ {
+ g_atomic_int_set (&cancellable_source->cancelled_callback_called, FALSE);
+ g_atomic_pointer_set (cancellable_source->self_ptr, source);
+ }
return FALSE;
}
@@ -715,7 +726,10 @@ cancellable_source_dispose (GSource *source)
if (cancellable)
{
- if (g_atomic_int_get (&cancellable->priv->cancelled_running))
+ GSource *self_ptr =
+ g_atomic_pointer_exchange (cancellable_source->self_ptr, NULL);
+
+ if (self_ptr == NULL)
{
/* There can be a race here: if thread A has called
* g_cancellable_cancel() and has got as far as committing to call
@@ -810,14 +824,17 @@ g_cancellable_source_new (GCancellable *cancellable)
if (cancellable)
{
cancellable_source->cancellable = g_object_ref (cancellable);
+ cancellable_source->self_ptr = g_new (GSource *, 1);
+ g_atomic_pointer_set (cancellable_source->self_ptr, source);
/* We intentionally don't use g_cancellable_connect() here,
* because we don't want the "at most once" behavior.
*/
cancellable_source->cancelled_handler =
- g_signal_connect (cancellable, "cancelled",
- G_CALLBACK (cancellable_source_cancelled),
- source);
+ g_signal_connect_data (cancellable, "cancelled",
+ G_CALLBACK (cancellable_source_cancelled),
+ cancellable_source->self_ptr,
+ (GClosureNotify) g_free, G_CONNECT_DEFAULT);
if (g_cancellable_is_cancelled (cancellable))
g_source_set_ready_time (source, 0);
}
From ff2c5c3e74d4c7e8c9d9c493cc2a0adb7997120e Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@gnome.org>
Date: Tue, 4 Feb 2025 14:06:15 +0000
Subject: [PATCH 2/2] tests: Fix non-atomic accesses to atomic variables in
cancellable test
As suggested by Marco Trevisan in !4206.
This might eliminate some spurious failures of the test.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
---
gio/tests/cancellable.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/gio/tests/cancellable.c b/gio/tests/cancellable.c
index fa84e2e2c..bbe53e4a2 100644
--- a/gio/tests/cancellable.c
+++ b/gio/tests/cancellable.c
@@ -710,7 +710,7 @@ static void
on_racy_cancellable_cancelled (GCancellable *cancellable,
gpointer data)
{
- gboolean *callback_called = data;
+ gboolean *callback_called = data; /* (atomic) */
g_assert_true (g_cancellable_is_cancelled (cancellable));
g_atomic_int_set (callback_called, TRUE);
@@ -722,7 +722,7 @@ test_cancellable_cancel_reset_races (void)
GCancellable *cancellable;
GThread *resetting_thread = NULL;
GThread *cancelling_thread = NULL;
- gboolean callback_called = FALSE;
+ gboolean callback_called = FALSE; /* (atomic) */
g_test_summary ("Tests threads racing for cancelling and resetting a GCancellable");
@@ -730,7 +730,7 @@ test_cancellable_cancel_reset_races (void)
g_cancellable_connect (cancellable, G_CALLBACK (on_racy_cancellable_cancelled),
&callback_called, NULL);
- g_assert_false (callback_called);
+ g_assert_false (g_atomic_int_get (&callback_called));
resetting_thread = g_thread_new ("/cancellable/cancel-reset-races/resetting",
repeatedly_resetting_thread,
@@ -741,7 +741,7 @@ test_cancellable_cancel_reset_races (void)
g_thread_join (g_steal_pointer (&cancelling_thread));
g_thread_join (g_steal_pointer (&resetting_thread));
- g_assert_true (callback_called);
+ g_assert_true (g_atomic_int_get (&callback_called));
g_object_unref (cancellable);
}
@@ -755,7 +755,7 @@ repeatedly_connecting_thread (gpointer data)
for (guint i = 0; i < iterations; ++i)
{
- gboolean callback_called = FALSE;
+ gboolean callback_called = FALSE; /* (atomic) */
gboolean called;
gulong id = g_cancellable_connect (cancellable,
G_CALLBACK (on_racy_cancellable_cancelled),
@@ -780,7 +780,7 @@ test_cancellable_cancel_reset_connect_races (void)
GThread *resetting_thread = NULL;
GThread *cancelling_thread = NULL;
GThread *connecting_thread = NULL;
- gboolean callback_called = FALSE;
+ gboolean callback_called = FALSE; /* (atomic) */
g_test_summary ("Tests threads racing for cancelling, connecting and disconnecting "
" and resetting a GCancellable");
@@ -789,7 +789,7 @@ test_cancellable_cancel_reset_connect_races (void)
g_cancellable_connect (cancellable, G_CALLBACK (on_racy_cancellable_cancelled),
&callback_called, NULL);
- g_assert_false (callback_called);
+ g_assert_false (g_atomic_int_get (&callback_called));
resetting_thread = g_thread_new ("/cancel-reset-connect-races/resetting",
repeatedly_resetting_thread,
@@ -803,7 +803,7 @@ test_cancellable_cancel_reset_connect_races (void)
g_thread_join (g_steal_pointer (&resetting_thread));
g_thread_join (g_steal_pointer (&connecting_thread));
- g_assert_true (callback_called);
+ g_assert_true (g_atomic_int_get (&callback_called));
g_object_unref (cancellable);
}