From d72192f69b92c20cd202c23cf75fe248ec2aa2dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Thu, 21 Feb 2019 00:00:00 +0000 Subject: [PATCH 1/8] gobject: Remove unsynchronized read of freeze_count There is no need to preserve the check, since check is performed again while holding the notify_locks that protects freeze_count. --- gobject/gobject.c | 1 - 1 file changed, 1 deletion(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index de61a0481..5f23a4908 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -263,7 +263,6 @@ g_object_notify_queue_thaw (GObject *object, GSList *slist; guint n_pspecs = 0; - g_return_if_fail (nqueue->freeze_count > 0); g_return_if_fail (g_atomic_int_get(&object->ref_count) > 0); G_LOCK(notify_lock); From fba7f7e097e787a4689bbcb41b0c5c51e900722e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Thu, 21 Feb 2019 00:00:00 +0000 Subject: [PATCH 2/8] gparam: Remove unsynchronized write to g_type field GValue g_type field is used for synchronization with g_once_init_enter, and so it should be written to only with g_once_init_leave. Replace structure copy with memcpy that copies the one remaining field of GValue, i.e., data array. --- gobject/gparam.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gobject/gparam.c b/gobject/gparam.c index 79ee834d5..6f33f027b 100644 --- a/gobject/gparam.c +++ b/gobject/gparam.c @@ -1561,8 +1561,7 @@ g_param_spec_get_default_value (GParamSpec *pspec) g_param_value_set_default (pspec, &default_value); /* store all but the type */ - default_value.g_type = 0; - priv->default_value = default_value; + memcpy (priv->default_value.data, default_value.data, sizeof (default_value.data)); g_once_init_leave (&priv->default_value.g_type, pspec->value_type); } From a3060bc84fc436abf4fcae80b0647e13672b41c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Thu, 21 Feb 2019 00:00:00 +0000 Subject: [PATCH 3/8] gmain: Synchronize access to is_running flag of GMainLoop Synchronize access to is_running field of GMainLoop to ensure that g_main_loop_is_running is thread safe. --- glib/gmain.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/glib/gmain.c b/glib/gmain.c index 83e398cfa..860c39dd1 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -309,7 +309,7 @@ struct _GSourceCallback struct _GMainLoop { GMainContext *context; - gboolean is_running; + gboolean is_running; /* (atomic) */ volatile gint ref_count; }; @@ -4087,16 +4087,14 @@ g_main_loop_run (GMainLoop *loop) LOCK_CONTEXT (loop->context); g_atomic_int_inc (&loop->ref_count); + g_atomic_int_set (&loop->is_running, TRUE); - if (!loop->is_running) - loop->is_running = TRUE; - - while (loop->is_running && !got_ownership) + while (g_atomic_int_get (&loop->is_running) && !got_ownership) got_ownership = g_main_context_wait_internal (loop->context, &loop->context->cond, &loop->context->mutex); - if (!loop->is_running) + if (!g_atomic_int_get (&loop->is_running)) { UNLOCK_CONTEXT (loop->context); if (got_ownership) @@ -4118,8 +4116,8 @@ g_main_loop_run (GMainLoop *loop) } g_atomic_int_inc (&loop->ref_count); - loop->is_running = TRUE; - while (loop->is_running) + g_atomic_int_set (&loop->is_running, TRUE); + while (g_atomic_int_get (&loop->is_running)) g_main_context_iterate (loop->context, TRUE, TRUE, self); UNLOCK_CONTEXT (loop->context); @@ -4146,7 +4144,7 @@ g_main_loop_quit (GMainLoop *loop) g_return_if_fail (g_atomic_int_get (&loop->ref_count) > 0); LOCK_CONTEXT (loop->context); - loop->is_running = FALSE; + g_atomic_int_set (&loop->is_running, FALSE); g_wakeup_signal (loop->context->wakeup); g_cond_broadcast (&loop->context->cond); @@ -4170,7 +4168,7 @@ g_main_loop_is_running (GMainLoop *loop) g_return_val_if_fail (loop != NULL, FALSE); g_return_val_if_fail (g_atomic_int_get (&loop->ref_count) > 0, FALSE); - return loop->is_running; + return g_atomic_int_get (&loop->is_running); } /** From c4cb27d844f99f5cc9fac3ac4c9109ce47ad9f0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Fri, 22 Feb 2019 00:00:00 +0000 Subject: [PATCH 4/8] gobject: Use atomic operations to read object reference count --- gobject/gobject.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 5f23a4908..22c2cb39e 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -1100,7 +1100,7 @@ void g_object_run_dispose (GObject *object) { g_return_if_fail (G_IS_OBJECT (object)); - g_return_if_fail (object->ref_count > 0); + g_return_if_fail (g_atomic_int_get (&object->ref_count) > 0); g_object_ref (object); TRACE (GOBJECT_OBJECT_DISPOSE(object,G_TYPE_FROM_INSTANCE(object), 0)); @@ -2818,7 +2818,7 @@ g_object_weak_ref (GObject *object, g_return_if_fail (G_IS_OBJECT (object)); g_return_if_fail (notify != NULL); - g_return_if_fail (object->ref_count >= 1); + g_return_if_fail (g_atomic_int_get (&object->ref_count) >= 1); G_LOCK (weak_refs_mutex); wstack = g_datalist_id_remove_no_notify (&object->qdata, quark_weak_refs); @@ -3000,7 +3000,7 @@ gpointer GObject *object = _object; gboolean was_floating; g_return_val_if_fail (G_IS_OBJECT (object), object); - g_return_val_if_fail (object->ref_count >= 1, object); + g_return_val_if_fail (g_atomic_int_get (&object->ref_count) >= 1, object); g_object_ref (object); was_floating = floating_flag_handler (object, -1); if (was_floating) @@ -3023,7 +3023,7 @@ void g_object_force_floating (GObject *object) { g_return_if_fail (G_IS_OBJECT (object)); - g_return_if_fail (object->ref_count >= 1); + g_return_if_fail (g_atomic_int_get (&object->ref_count) >= 1); floating_flag_handler (object, +1); } @@ -3104,7 +3104,7 @@ g_object_add_toggle_ref (GObject *object, g_return_if_fail (G_IS_OBJECT (object)); g_return_if_fail (notify != NULL); - g_return_if_fail (object->ref_count >= 1); + g_return_if_fail (g_atomic_int_get (&object->ref_count) >= 1); g_object_ref (object); @@ -4138,7 +4138,7 @@ g_object_watch_closure (GObject *object, g_return_if_fail (closure != NULL); g_return_if_fail (closure->is_invalid == FALSE); g_return_if_fail (closure->in_marshal == FALSE); - g_return_if_fail (object->ref_count > 0); /* this doesn't work on finalizing objects */ + g_return_if_fail (g_atomic_int_get (&object->ref_count) > 0); /* this doesn't work on finalizing objects */ g_closure_add_invalidate_notifier (closure, object, object_remove_closure); g_closure_add_marshal_guards (closure, @@ -4184,7 +4184,7 @@ g_closure_new_object (guint sizeof_closure, GClosure *closure; g_return_val_if_fail (G_IS_OBJECT (object), NULL); - g_return_val_if_fail (object->ref_count > 0, NULL); /* this doesn't work on finalizing objects */ + g_return_val_if_fail (g_atomic_int_get (&object->ref_count) > 0, NULL); /* this doesn't work on finalizing objects */ closure = g_closure_new_simple (sizeof_closure, object); g_object_watch_closure (object, closure); @@ -4212,7 +4212,7 @@ g_cclosure_new_object (GCallback callback_func, GClosure *closure; g_return_val_if_fail (G_IS_OBJECT (object), NULL); - g_return_val_if_fail (object->ref_count > 0, NULL); /* this doesn't work on finalizing objects */ + g_return_val_if_fail (g_atomic_int_get (&object->ref_count) > 0, NULL); /* this doesn't work on finalizing objects */ g_return_val_if_fail (callback_func != NULL, NULL); closure = g_cclosure_new (callback_func, object, NULL); @@ -4241,7 +4241,7 @@ g_cclosure_new_object_swap (GCallback callback_func, GClosure *closure; g_return_val_if_fail (G_IS_OBJECT (object), NULL); - g_return_val_if_fail (object->ref_count > 0, NULL); /* this doesn't work on finalizing objects */ + g_return_val_if_fail (g_atomic_int_get (&object->ref_count) > 0, NULL); /* this doesn't work on finalizing objects */ g_return_val_if_fail (callback_func != NULL, NULL); closure = g_cclosure_new_swap (callback_func, object, NULL); From 6336864171d0d14297de5af2aba0a5de00a8d257 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Fri, 22 Feb 2019 00:00:00 +0000 Subject: [PATCH 5/8] glocalfilemonitor: Fix data race in local file monitor Ensure that source is attached to the context before it migth be used from another thread, since otherwise operation on source are unsynchronized and not thread-safe. In particular there was a data race between g_source_attach and g_source_set_ready_time (used from g_file_monitor_source_handle_event). --- gio/glocalfilemonitor.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gio/glocalfilemonitor.c b/gio/glocalfilemonitor.c index 8a4c4ec43..278d9a492 100644 --- a/gio/glocalfilemonitor.c +++ b/gio/glocalfilemonitor.c @@ -788,11 +788,11 @@ g_local_file_monitor_start (GLocalFileMonitor *local_monitor, #endif } + g_source_attach ((GSource *) source, context); + G_LOCAL_FILE_MONITOR_GET_CLASS (local_monitor)->start (local_monitor, source->dirname, source->basename, source->filename, source); - - g_source_attach ((GSource *) source, context); } static void From f975858e866cc6420b00d29d769af0fb1ad3e7e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Mon, 25 Feb 2019 00:00:00 +0000 Subject: [PATCH 6/8] gcancellable: Synchronize access to cancelled flag Synchronize access to cancelled flag of cancellable, which was previously access without synchronization in g_cancellable_is_cancelled. Use atomic operations instead of existing global mutex, to avoid serializing calls to g_cancellable_is_cancelled across all threads. --- gio/gcancellable.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/gio/gcancellable.c b/gio/gcancellable.c index de6d43495..32605ce02 100644 --- a/gio/gcancellable.c +++ b/gio/gcancellable.c @@ -43,7 +43,9 @@ enum { struct _GCancellablePrivate { - guint cancelled : 1; + /* Atomic so that g_cancellable_is_cancelled does not require holding the mutex. */ + gboolean cancelled; + /* Access to fields below is protected by cancellable_mutex. */ guint cancelled_running : 1; guint cancelled_running_waiting : 1; @@ -269,12 +271,12 @@ g_cancellable_reset (GCancellable *cancellable) g_cond_wait (&cancellable_cond, &cancellable_mutex); } - if (priv->cancelled) + if (g_atomic_int_get (&priv->cancelled)) { if (priv->wakeup) GLIB_PRIVATE_CALL (g_wakeup_acknowledge) (priv->wakeup); - priv->cancelled = FALSE; + g_atomic_int_set (&priv->cancelled, FALSE); } g_mutex_unlock (&cancellable_mutex); @@ -292,7 +294,7 @@ g_cancellable_reset (GCancellable *cancellable) gboolean g_cancellable_is_cancelled (GCancellable *cancellable) { - return cancellable != NULL && cancellable->priv->cancelled; + return cancellable != NULL && g_atomic_int_get (&cancellable->priv->cancelled); } /** @@ -404,7 +406,7 @@ g_cancellable_make_pollfd (GCancellable *cancellable, GPollFD *pollfd) { cancellable->priv->wakeup = GLIB_PRIVATE_CALL (g_wakeup_new) (); - if (cancellable->priv->cancelled) + if (g_atomic_int_get (&cancellable->priv->cancelled)) GLIB_PRIVATE_CALL (g_wakeup_signal) (cancellable->priv->wakeup); } @@ -440,11 +442,11 @@ g_cancellable_release_fd (GCancellable *cancellable) return; g_return_if_fail (G_IS_CANCELLABLE (cancellable)); - g_return_if_fail (cancellable->priv->fd_refcount > 0); priv = cancellable->priv; g_mutex_lock (&cancellable_mutex); + g_assert (priv->fd_refcount > 0); priv->fd_refcount--; if (priv->fd_refcount == 0) @@ -482,21 +484,20 @@ g_cancellable_cancel (GCancellable *cancellable) { GCancellablePrivate *priv; - if (cancellable == NULL || - cancellable->priv->cancelled) + if (g_cancellable_is_cancelled (cancellable)) return; priv = cancellable->priv; g_mutex_lock (&cancellable_mutex); - if (priv->cancelled) + if (g_atomic_int_get (&priv->cancelled)) { g_mutex_unlock (&cancellable_mutex); return; } - priv->cancelled = TRUE; + g_atomic_int_set (&priv->cancelled, TRUE); priv->cancelled_running = TRUE; if (priv->wakeup) @@ -562,7 +563,7 @@ g_cancellable_connect (GCancellable *cancellable, g_mutex_lock (&cancellable_mutex); - if (cancellable->priv->cancelled) + if (g_atomic_int_get (&cancellable->priv->cancelled)) { void (*_callback) (GCancellable *cancellable, gpointer user_data); From d75605e866e1049223c88ffdbd2f78be07423812 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Fri, 22 Feb 2019 00:00:00 +0000 Subject: [PATCH 7/8] tests: Synchronize access to stopping flag --- tests/refcount/properties.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/refcount/properties.c b/tests/refcount/properties.c index c68234216..376d9313c 100644 --- a/tests/refcount/properties.c +++ b/tests/refcount/properties.c @@ -35,7 +35,7 @@ struct _GTestClass }; static GType my_test_get_type (void); -static volatile gboolean stopping; +static gboolean stopping; static void my_test_class_init (GTestClass * klass); static void my_test_init (GTest * test); @@ -177,7 +177,7 @@ run_thread (GTest * test) { gint i = 1; - while (!stopping) { + while (!g_atomic_int_get (&stopping)) { my_test_do_property (test); if ((i++ % 10000) == 0) { @@ -210,14 +210,14 @@ main (int argc, char **argv) g_signal_connect (test, "notify::dummy", G_CALLBACK (dummy_notify), NULL); } - stopping = FALSE; + g_atomic_int_set (&stopping, FALSE); for (i = 0; i < N_THREADS; i++) test_threads[i] = g_thread_create ((GThreadFunc) run_thread, test_objects[i], TRUE, NULL); g_usleep (3000000); - stopping = TRUE; + g_atomic_int_set (&stopping, TRUE); g_print ("\nstopping\n"); /* join all threads */ From c52021f34026c3b2d0649920a2a8c342e660085a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Fri, 22 Feb 2019 00:00:00 +0000 Subject: [PATCH 8/8] tests: Don't leak check-proxies thread --- gio/tests/gdbus-test-codegen.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/gio/tests/gdbus-test-codegen.c b/gio/tests/gdbus-test-codegen.c index 8db555781..985cb1095 100644 --- a/gio/tests/gdbus-test-codegen.c +++ b/gio/tests/gdbus-test-codegen.c @@ -583,10 +583,8 @@ on_name_acquired (GDBusConnection *connection, gpointer user_data) { GMainLoop *loop = user_data; - - g_thread_new ("check-proxies", - check_proxies_in_thread, - loop); + GThread *thread = g_thread_new ("check-proxies", check_proxies_in_thread, loop); + g_thread_unref (thread); } static void