From 497a8c93cb1f340b41284e4f64709577b9640f57 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] 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 adding a new private flag (so that the public one can still be used as it used to, without having to use atomic APIs there) that is now used to atomically change and read the values. And this fixes various tests in thread sanitizer --- glib/gmain.c | 57 ++++++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/glib/gmain.c b/glib/gmain.c index 73b4ed93b..4d5a07d1d 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -290,6 +290,8 @@ struct _GSourcePrivate */ GSList *fds; + GSourceFlags flags; /* atomic */ + GSourceDisposeFunc dispose; gboolean static_name; @@ -307,8 +309,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)->priv->flags)) & G_HOOK_FLAG_ACTIVE) == 0) +#define SOURCE_BLOCKED(source) \ + ((g_atomic_int_get (&((source)->priv->flags)) & G_SOURCE_BLOCKED) != 0) /* Forward declarations */ @@ -934,7 +938,7 @@ g_source_new (GSourceFuncs *source_funcs, source->priority = G_PRIORITY_DEFAULT; - source->flags = G_HOOK_FLAG_ACTIVE; + g_atomic_int_set (&source->priv->flags, G_HOOK_FLAG_ACTIVE); source->priv->ready_time = -1; @@ -1284,8 +1288,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->priv->flags, ~G_HOOK_FLAG_ACTIVE); old_cb_data = source->callback_data; old_cb_funcs = source->callback_funcs; @@ -1359,7 +1363,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->priv->flags, ~G_HOOK_FLAG_ACTIVE); } /** @@ -2007,9 +2011,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->priv->flags, G_SOURCE_CAN_RECURSE); else - source->flags &= ~G_SOURCE_CAN_RECURSE; + g_atomic_int_and (&source->priv->flags, ~G_SOURCE_CAN_RECURSE); if (context) UNLOCK_CONTEXT (context); @@ -2029,8 +2033,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->priv->flags) & G_SOURCE_CAN_RECURSE) != 0; } static void @@ -3240,7 +3244,7 @@ block_source (GSource *source) g_return_if_fail (!SOURCE_BLOCKED (source)); - source->flags |= G_SOURCE_BLOCKED; + g_atomic_int_or (&source->priv->flags, G_SOURCE_BLOCKED); if (source->context) { @@ -3274,8 +3278,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->priv->flags, ~G_SOURCE_BLOCKED); tmp_list = source->poll_fds; while (tmp_list) @@ -3312,7 +3316,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->priv->flags, ~G_SOURCE_READY); if (!SOURCE_DESTROYED (source)) { @@ -3336,11 +3340,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->priv->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->priv->flags, + (GSourceFlags) G_HOOK_FLAG_IN_CALL) & + G_HOOK_FLAG_IN_CALL; if (cb_funcs) cb_funcs->get (cb_data, source, &callback, &user_data); @@ -3377,9 +3382,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->priv->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 @@ -3746,7 +3751,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->priv->flags) & G_SOURCE_READY)) { gboolean result; gboolean (* prepare) (GSource *source, @@ -3807,13 +3812,13 @@ g_main_context_prepare_unlocked (GMainContext *context, while (ready_source) { - ready_source->flags |= G_SOURCE_READY; + g_atomic_int_or (&ready_source->priv->flags, G_SOURCE_READY); ready_source = ready_source->priv->parent_source; } } } - if (source->flags & G_SOURCE_READY) + if (g_atomic_int_get (&source->priv->flags) & G_SOURCE_READY) { n_ready++; current_priority = source->priority; @@ -4079,8 +4084,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->priv->flags) & G_SOURCE_READY)) + { gboolean result; gboolean (* check) (GSource *source); @@ -4150,13 +4155,13 @@ g_main_context_check_unlocked (GMainContext *context, while (ready_source) { - ready_source->flags |= G_SOURCE_READY; + g_atomic_int_or (&ready_source->priv->flags, G_SOURCE_READY); ready_source = ready_source->priv->parent_source; } } } - if (source->flags & G_SOURCE_READY) + if (g_atomic_int_get (&source->priv->flags) & G_SOURCE_READY) { g_source_ref (source); g_ptr_array_add (context->pending_dispatches, source);