From a5bc497021a71a68df91109630e73a7c436cabb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 31 Jan 2025 11:23:53 +0100 Subject: [PATCH 1/4] gmain: Use atomic logic to handle internal GSource flags We use flags in both locked paths and in public ones (to check if a source is destroyed or running), but those checks are not using atomic logic, thus they lead to races in various tests. Fix them by atomically change and read the values. And this fixes various tests in thread sanitizer. --- glib/gmain.c | 55 +++++++++++++++++++++++++++------------------------- glib/gmain.h | 2 +- 2 files changed, 30 insertions(+), 27 deletions(-) diff --git a/glib/gmain.c b/glib/gmain.c index c89ffdc73..35c9d23d5 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -307,8 +307,10 @@ typedef struct _GSourceIter #define UNLOCK_CONTEXT(context) g_mutex_unlock (&context->mutex) #define G_THREAD_SELF g_thread_self () -#define SOURCE_DESTROYED(source) (((source)->flags & G_HOOK_FLAG_ACTIVE) == 0) -#define SOURCE_BLOCKED(source) (((source)->flags & G_SOURCE_BLOCKED) != 0) +#define SOURCE_DESTROYED(source) \ + ((g_atomic_int_get (&((source)->flags)) & G_HOOK_FLAG_ACTIVE) == 0) +#define SOURCE_BLOCKED(source) \ + ((g_atomic_int_get (&((source)->flags)) & G_SOURCE_BLOCKED) != 0) /* Forward declarations */ @@ -934,7 +936,7 @@ g_source_new (GSourceFuncs *source_funcs, source->priority = G_PRIORITY_DEFAULT; - source->flags = G_HOOK_FLAG_ACTIVE; + g_atomic_int_set (&source->flags, G_HOOK_FLAG_ACTIVE); source->priv->ready_time = -1; @@ -1284,8 +1286,8 @@ g_source_destroy_internal (GSource *source, GSList *tmp_list; gpointer old_cb_data; GSourceCallbackFuncs *old_cb_funcs; - - source->flags &= ~G_HOOK_FLAG_ACTIVE; + + g_atomic_int_and (&source->flags, ~G_HOOK_FLAG_ACTIVE); old_cb_data = source->callback_data; old_cb_funcs = source->callback_funcs; @@ -1359,7 +1361,7 @@ g_source_destroy (GSource *source) if (context) g_source_destroy_internal (source, context, FALSE); else - source->flags &= ~G_HOOK_FLAG_ACTIVE; + g_atomic_int_and (&source->flags, ~G_HOOK_FLAG_ACTIVE); } /** @@ -2007,9 +2009,9 @@ g_source_set_can_recurse (GSource *source, LOCK_CONTEXT (context); if (can_recurse) - source->flags |= G_SOURCE_CAN_RECURSE; + g_atomic_int_or (&source->flags, G_SOURCE_CAN_RECURSE); else - source->flags &= ~G_SOURCE_CAN_RECURSE; + g_atomic_int_and (&source->flags, ~G_SOURCE_CAN_RECURSE); if (context) UNLOCK_CONTEXT (context); @@ -2029,8 +2031,8 @@ g_source_get_can_recurse (GSource *source) { g_return_val_if_fail (source != NULL, FALSE); g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, FALSE); - - return (source->flags & G_SOURCE_CAN_RECURSE) != 0; + + return (g_atomic_int_get (&source->flags) & G_SOURCE_CAN_RECURSE) != 0; } static void @@ -3267,7 +3269,7 @@ block_source (GSource *source) g_return_if_fail (!SOURCE_BLOCKED (source)); - source->flags |= G_SOURCE_BLOCKED; + g_atomic_int_or (&source->flags, G_SOURCE_BLOCKED); if (source->context) { @@ -3301,8 +3303,8 @@ unblock_source (GSource *source) g_return_if_fail (SOURCE_BLOCKED (source)); /* Source already unblocked */ g_return_if_fail (!SOURCE_DESTROYED (source)); - - source->flags &= ~G_SOURCE_BLOCKED; + + g_atomic_int_and (&source->flags, ~G_SOURCE_BLOCKED); tmp_list = source->poll_fds; while (tmp_list) @@ -3339,7 +3341,7 @@ g_main_dispatch (GMainContext *context) context->pending_dispatches->pdata[i] = NULL; g_assert (source); - source->flags &= ~G_SOURCE_READY; + g_atomic_int_and (&source->flags, ~G_SOURCE_READY); if (!SOURCE_DESTROYED (source)) { @@ -3363,11 +3365,12 @@ g_main_dispatch (GMainContext *context) if (cb_funcs) cb_funcs->ref (cb_data); - if ((source->flags & G_SOURCE_CAN_RECURSE) == 0) + if ((g_atomic_int_get (&source->flags) & G_SOURCE_CAN_RECURSE) == 0) block_source (source); - was_in_call = source->flags & G_HOOK_FLAG_IN_CALL; - source->flags |= G_HOOK_FLAG_IN_CALL; + was_in_call = g_atomic_int_or (&source->flags, + (GSourceFlags) G_HOOK_FLAG_IN_CALL) & + G_HOOK_FLAG_IN_CALL; if (cb_funcs) cb_funcs->get (cb_data, source, &callback, &user_data); @@ -3404,9 +3407,9 @@ g_main_dispatch (GMainContext *context) LOCK_CONTEXT (context); if (!was_in_call) - source->flags &= ~G_HOOK_FLAG_IN_CALL; + g_atomic_int_and (&source->flags, ~G_HOOK_FLAG_IN_CALL); - if (SOURCE_BLOCKED (source) && !SOURCE_DESTROYED (source)) + if (SOURCE_BLOCKED (source) && !SOURCE_DESTROYED (source)) unblock_source (source); /* Note: this depends on the fact that we can't switch @@ -3773,7 +3776,7 @@ g_main_context_prepare_unlocked (GMainContext *context, if ((n_ready > 0) && (source->priority > current_priority)) break; - if (!(source->flags & G_SOURCE_READY)) + if (!(g_atomic_int_get (&source->flags) & G_SOURCE_READY)) { gboolean result; gboolean (* prepare) (GSource *source, @@ -3834,13 +3837,13 @@ g_main_context_prepare_unlocked (GMainContext *context, while (ready_source) { - ready_source->flags |= G_SOURCE_READY; + g_atomic_int_or (&ready_source->flags, G_SOURCE_READY); ready_source = ready_source->priv->parent_source; } } } - if (source->flags & G_SOURCE_READY) + if (g_atomic_int_get (&source->flags) & G_SOURCE_READY) { n_ready++; current_priority = source->priority; @@ -4106,8 +4109,8 @@ g_main_context_check_unlocked (GMainContext *context, if ((n_ready > 0) && (source->priority > max_priority)) break; - if (!(source->flags & G_SOURCE_READY)) - { + if (!(g_atomic_int_get (&source->flags) & G_SOURCE_READY)) + { gboolean result; gboolean (* check) (GSource *source); @@ -4177,13 +4180,13 @@ g_main_context_check_unlocked (GMainContext *context, while (ready_source) { - ready_source->flags |= G_SOURCE_READY; + g_atomic_int_or (&ready_source->flags, G_SOURCE_READY); ready_source = ready_source->priv->parent_source; } } } - if (source->flags & G_SOURCE_READY) + if (g_atomic_int_get (&source->flags) & G_SOURCE_READY) { g_source_ref (source); g_ptr_array_add (context->pending_dispatches, source); diff --git a/glib/gmain.h b/glib/gmain.h index 0c333c0cd..d8a9e21e4 100644 --- a/glib/gmain.h +++ b/glib/gmain.h @@ -275,7 +275,7 @@ struct _GSource GMainContext *context; gint priority; - guint flags; + guint flags; /* (atomic) */ guint source_id; GSList *poll_fds; From 42283ffb455ffb000b4f77453a6d55f82350161f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 31 Jan 2025 13:04:47 +0100 Subject: [PATCH 2/4] gmain: Use atomic logic to set and use dispose function It can be read/written in a racy way, so let's be safer. --- glib/gmain.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/glib/gmain.c b/glib/gmain.c index 35c9d23d5..4e94509ff 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -32,6 +32,7 @@ */ #include "config.h" +#include "glib.h" #include "glibconfig.h" #include "glib_trace.h" @@ -978,10 +979,14 @@ void g_source_set_dispose_function (GSource *source, GSourceDisposeFunc dispose) { + gboolean was_unset G_GNUC_UNUSED; + g_return_if_fail (source != NULL); - g_return_if_fail (source->priv->dispose == NULL); g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0); - source->priv->dispose = dispose; + + was_unset = g_atomic_pointer_compare_and_exchange (&source->priv->dispose, + NULL, dispose); + g_return_if_fail (was_unset); } /* Holds context's lock */ @@ -2248,11 +2253,13 @@ retry_beginning: if (old_ref == 1) { /* If there's a dispose function, call this first */ - if (source->priv->dispose) + GSourceDisposeFunc dispose_func; + + if ((dispose_func = g_atomic_pointer_get (&source->priv->dispose))) { if (context) UNLOCK_CONTEXT (context); - source->priv->dispose (source); + dispose_func (source); if (context) LOCK_CONTEXT (context); } From ee836d77230f94e6b02442cc3f7901cdc4cff8f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 7 Feb 2025 18:48:22 +0100 Subject: [PATCH 3/4] gmain: Use atomic logic to init the ref counting Ensure we're consistent with the field usage --- glib/gmain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/gmain.c b/glib/gmain.c index 4e94509ff..2a68313a2 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -933,7 +933,7 @@ g_source_new (GSourceFuncs *source_funcs, source = (GSource*) g_malloc0 (struct_size); source->priv = g_slice_new0 (GSourcePrivate); source->source_funcs = source_funcs; - source->ref_count = 1; + g_atomic_int_set (&source->ref_count, 1); source->priority = G_PRIORITY_DEFAULT; From 14da9d459ceb37ecb2ee4e6003cabf2fff319f7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 19 Feb 2025 05:34:13 +0100 Subject: [PATCH 4/4] gio/tests/cancellable: Actually init atomic values with atomic functions This is to make thread sanitizer happy, even those aren't really issues. --- gio/tests/cancellable.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/gio/tests/cancellable.c b/gio/tests/cancellable.c index bbe53e4a2..f8ff69cb5 100644 --- a/gio/tests/cancellable.c +++ b/gio/tests/cancellable.c @@ -755,8 +755,11 @@ repeatedly_connecting_thread (gpointer data) for (guint i = 0; i < iterations; ++i) { - gboolean callback_called = FALSE; /* (atomic) */ + gboolean callback_called; /* (atomic) */ gboolean called; + + g_atomic_int_set (&callback_called, FALSE); + gulong id = g_cancellable_connect (cancellable, G_CALLBACK (on_racy_cancellable_cancelled), &callback_called, NULL); @@ -780,13 +783,14 @@ test_cancellable_cancel_reset_connect_races (void) GThread *resetting_thread = NULL; GThread *cancelling_thread = NULL; GThread *connecting_thread = NULL; - gboolean callback_called = FALSE; /* (atomic) */ + gboolean callback_called; /* (atomic) */ g_test_summary ("Tests threads racing for cancelling, connecting and disconnecting " " and resetting a GCancellable"); cancellable = g_cancellable_new (); + g_atomic_int_set (&callback_called, FALSE); g_cancellable_connect (cancellable, G_CALLBACK (on_racy_cancellable_cancelled), &callback_called, NULL); g_assert_false (g_atomic_int_get (&callback_called));