From ec569ff2e2f723164602692d3b695b117f3b3e43 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2019 15:17:35 +0100 Subject: [PATCH 1/7] gthreadedsocketservice: Tidy up property declarations This introduces no functional changes. Signed-off-by: Philip Withnall --- gio/gthreadedsocketservice.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/gio/gthreadedsocketservice.c b/gio/gthreadedsocketservice.c index e0c111c10..7644d942c 100644 --- a/gio/gthreadedsocketservice.c +++ b/gio/gthreadedsocketservice.c @@ -62,11 +62,10 @@ G_DEFINE_TYPE_WITH_PRIVATE (GThreadedSocketService, g_threaded_socket_service, G_TYPE_SOCKET_SERVICE) -enum +typedef enum { - PROP_0, - PROP_MAX_THREADS -}; + PROP_MAX_THREADS = 1, +} GThreadedSocketServiceProperty; G_LOCK_DEFINE_STATIC(job_count); @@ -173,7 +172,7 @@ g_threaded_socket_service_get_property (GObject *object, { GThreadedSocketService *service = G_THREADED_SOCKET_SERVICE (object); - switch (prop_id) + switch ((GThreadedSocketServiceProperty) prop_id) { case PROP_MAX_THREADS: g_value_set_int (value, service->priv->max_threads); @@ -192,7 +191,7 @@ g_threaded_socket_service_set_property (GObject *object, { GThreadedSocketService *service = G_THREADED_SOCKET_SERVICE (object); - switch (prop_id) + switch ((GThreadedSocketServiceProperty) prop_id) { case PROP_MAX_THREADS: service->priv->max_threads = g_value_get_int (value); From 63823ae6287427e8ca0560c23f16ab0f5507559d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2019 15:21:10 +0100 Subject: [PATCH 2/7] gthreadedsocketservice: Abstract out a free function This introduces no functional changes. Signed-off-by: Philip Withnall --- gio/gthreadedsocketservice.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/gio/gthreadedsocketservice.c b/gio/gthreadedsocketservice.c index 7644d942c..394618f20 100644 --- a/gio/gthreadedsocketservice.c +++ b/gio/gthreadedsocketservice.c @@ -75,6 +75,14 @@ typedef struct GObject *source_object; } GThreadedSocketServiceData; +static void +g_threaded_socket_service_data_free (GThreadedSocketServiceData *data) +{ + g_clear_object (&data->connection); + g_clear_object (&data->source_object); + g_slice_free (GThreadedSocketServiceData, data); +} + static void g_threaded_socket_service_func (gpointer _data, gpointer user_data) @@ -86,17 +94,12 @@ g_threaded_socket_service_func (gpointer _data, g_signal_emit (threaded, g_threaded_socket_service_run_signal, 0, data->connection, data->source_object, &result); - g_object_unref (data->connection); - if (data->source_object) - g_object_unref (data->source_object); - g_slice_free (GThreadedSocketServiceData, data); - G_LOCK (job_count); if (threaded->priv->job_count-- == threaded->priv->max_threads) g_socket_service_start (G_SOCKET_SERVICE (threaded)); G_UNLOCK (job_count); - g_object_unref (threaded); + g_threaded_socket_service_data_free (data); } static gboolean From 035c5d03f184e0732279efc8384fc419ff71f649 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2019 15:22:19 +0100 Subject: [PATCH 3/7] gthreadedsocketservice: Move obj reference to per-job data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rather than keeping a reference to the GThreadedSocketService as the user_data for every thread pool job, add it to a member of the per-job data struct (GThreadedSocketServiceData). This should make no difference overall, as it’s just moving the refcounting around, but it does seem to fix an occasional double-unref crash on shutdown where the GThreadedSocketService is unreffed during finalisation. In any case, it makes the object ownership clearer. Signed-off-by: Philip Withnall --- gio/gthreadedsocketservice.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/gio/gthreadedsocketservice.c b/gio/gthreadedsocketservice.c index 394618f20..50ea4dd04 100644 --- a/gio/gthreadedsocketservice.c +++ b/gio/gthreadedsocketservice.c @@ -71,32 +71,33 @@ G_LOCK_DEFINE_STATIC(job_count); typedef struct { - GSocketConnection *connection; - GObject *source_object; + GThreadedSocketService *service; /* (owned) */ + GSocketConnection *connection; /* (owned) */ + GObject *source_object; /* (owned) (nullable) */ } GThreadedSocketServiceData; static void g_threaded_socket_service_data_free (GThreadedSocketServiceData *data) { + g_clear_object (&data->service); g_clear_object (&data->connection); g_clear_object (&data->source_object); g_slice_free (GThreadedSocketServiceData, data); } static void -g_threaded_socket_service_func (gpointer _data, - gpointer user_data) +g_threaded_socket_service_func (gpointer job_data, + gpointer user_data) { - GThreadedSocketService *threaded = user_data; - GThreadedSocketServiceData *data = _data; + GThreadedSocketServiceData *data = job_data; gboolean result; - g_signal_emit (threaded, g_threaded_socket_service_run_signal, + g_signal_emit (data->service, g_threaded_socket_service_run_signal, 0, data->connection, data->source_object, &result); G_LOCK (job_count); - if (threaded->priv->job_count-- == threaded->priv->max_threads) - g_socket_service_start (G_SOCKET_SERVICE (threaded)); + if (data->service->priv->job_count-- == data->service->priv->max_threads) + g_socket_service_start (G_SOCKET_SERVICE (data->service)); G_UNLOCK (job_count); g_threaded_socket_service_data_free (data); @@ -112,16 +113,10 @@ g_threaded_socket_service_incoming (GSocketService *service, threaded = G_THREADED_SOCKET_SERVICE (service); - data = g_slice_new (GThreadedSocketServiceData); - - /* Ref the socket service for the thread */ - g_object_ref (service); - + data = g_slice_new0 (GThreadedSocketServiceData); + data->service = g_object_ref (threaded); data->connection = g_object_ref (connection); - if (source_object) - data->source_object = g_object_ref (source_object); - else - data->source_object = NULL; + data->source_object = (source_object != NULL) ? g_object_ref (source_object) : NULL; G_LOCK (job_count); if (++threaded->priv->job_count == threaded->priv->max_threads) @@ -149,7 +144,7 @@ g_threaded_socket_service_constructed (GObject *object) service->priv->thread_pool = g_thread_pool_new (g_threaded_socket_service_func, - service, + NULL, service->priv->max_threads, FALSE, NULL); @@ -161,6 +156,8 @@ g_threaded_socket_service_finalize (GObject *object) { GThreadedSocketService *service = G_THREADED_SOCKET_SERVICE (object); + /* All jobs in the pool hold a reference to this #GThreadedSocketService, so + * this should only be called once the pool is empty: */ g_thread_pool_free (service->priv->thread_pool, FALSE, FALSE); G_OBJECT_CLASS (g_threaded_socket_service_parent_class) From 2dac14829936cae27ac3a9046b6fee058ebda7ba Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2019 15:24:05 +0100 Subject: [PATCH 4/7] gthreadedsocketservice: Handle thread pool push failure This was previously silently ignored, and would result in a leak. Signed-off-by: Philip Withnall --- gio/gthreadedsocketservice.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/gio/gthreadedsocketservice.c b/gio/gthreadedsocketservice.c index 50ea4dd04..b330196e3 100644 --- a/gio/gthreadedsocketservice.c +++ b/gio/gthreadedsocketservice.c @@ -110,6 +110,7 @@ g_threaded_socket_service_incoming (GSocketService *service, { GThreadedSocketService *threaded; GThreadedSocketServiceData *data; + GError *local_error = NULL; threaded = G_THREADED_SOCKET_SERVICE (service); @@ -123,9 +124,13 @@ g_threaded_socket_service_incoming (GSocketService *service, g_socket_service_stop (service); G_UNLOCK (job_count); - g_thread_pool_push (threaded->priv->thread_pool, data, NULL); - + if (!g_thread_pool_push (threaded->priv->thread_pool, data, &local_error)) + { + g_warning ("Error handling incoming socket: %s", local_error->message); + g_threaded_socket_service_data_free (data); + } + g_clear_error (&local_error); return FALSE; } From 96aa2e34b3a1c5aa91549a0568d4043b1b69c4af Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2019 16:55:17 +0100 Subject: [PATCH 5/7] gdbusdaemon: Fix error handling for filtering outgoing messages If the filter function for an outgoing message fails to copy the GDBusMessage, that failure was previously ignored, and GDBusMessage methods could be called on a NULL instance. Avoid that. Signed-off-by: Philip Withnall --- gio/gdbusdaemon.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/gio/gdbusdaemon.c b/gio/gdbusdaemon.c index e6b3c1af0..a9b626215 100644 --- a/gio/gdbusdaemon.c +++ b/gio/gdbusdaemon.c @@ -1493,16 +1493,21 @@ filter_function (GDBusConnection *connection, } else { + if (g_dbus_message_get_sender (message) == NULL || + g_dbus_message_get_destination (message) == NULL) + { + message = copy_if_locked (message); + if (message == NULL) + { + g_warning ("Failed to copy outgoing message"); + return NULL; + } + } + if (g_dbus_message_get_sender (message) == NULL) - { - message = copy_if_locked (message); - g_dbus_message_set_sender (message, DBUS_SERVICE_NAME); - } + g_dbus_message_set_sender (message, DBUS_SERVICE_NAME); if (g_dbus_message_get_destination (message) == NULL) - { - message = copy_if_locked (message); - g_dbus_message_set_destination (message, client->id); - } + g_dbus_message_set_destination (message, client->id); } return message; From 1fbf82be1720d4187097834b35c5ecc8f244bde0 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2019 16:56:04 +0100 Subject: [PATCH 6/7] gdbusprivate: Clarify GDBusMessage ownership transfers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add g_steal_pointer() and g_clear_object() calls in various places to clarify the ownership transfers for GDBusMessage instances, in a bid to understand what’s going on in this code and to try to find a use-after-finalize problem. This introduces no functional changes, but hopefully makes the code a little clearer. Signed-off-by: Philip Withnall --- gio/gdbusprivate.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c index dee1f2719..488fe50c4 100644 --- a/gio/gdbusprivate.c +++ b/gio/gdbusprivate.c @@ -492,9 +492,9 @@ _g_dbus_worker_emit_message_about_to_be_sent (GDBusWorker *worker, { GDBusMessage *ret; if (!g_atomic_int_get (&worker->stopped)) - ret = worker->message_about_to_be_sent_callback (worker, message, worker->user_data); + ret = worker->message_about_to_be_sent_callback (worker, g_steal_pointer (&message), worker->user_data); else - ret = message; + ret = g_steal_pointer (&message); return ret; } @@ -506,13 +506,13 @@ _g_dbus_worker_queue_or_deliver_received_message (GDBusWorker *worker, if (worker->frozen || g_queue_get_length (worker->received_messages_while_frozen) > 0) { /* queue up */ - g_queue_push_tail (worker->received_messages_while_frozen, message); + g_queue_push_tail (worker->received_messages_while_frozen, g_steal_pointer (&message)); } else { /* not frozen, nor anything in queue */ _g_dbus_worker_emit_message_received (worker, message); - g_object_unref (message); + g_clear_object (&message); } } @@ -529,7 +529,7 @@ unfreeze_in_idle_cb (gpointer user_data) while ((message = g_queue_pop_head (worker->received_messages_while_frozen)) != NULL) { _g_dbus_worker_emit_message_received (worker, message); - g_object_unref (message); + g_clear_object (&message); } worker->frozen = FALSE; } @@ -796,7 +796,7 @@ _g_dbus_worker_do_read_cb (GInputStream *input_stream, } /* yay, got a message, go deliver it */ - _g_dbus_worker_queue_or_deliver_received_message (worker, message); + _g_dbus_worker_queue_or_deliver_received_message (worker, g_steal_pointer (&message)); /* start reading another message! */ worker->read_buffer_bytes_wanted = 0; From bc81b2c4cbdcc24af02105910508b1580e264a9a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2019 17:08:22 +0100 Subject: [PATCH 7/7] tests: Unmark several gdbus-* tests as flaky MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After repeated local testing, I can’t reproduce failures with them: meson test --repeat 5000 gdbus-auth meson test --repeat 5000 gdbus-bz627724 meson test --repeat 5000 gdbus-connection The FreeBSD failures from pthread calls mentioned in #1614 should probably manifest as use-after-free for GMutex or pthread_mutex_t on Linux. Failing that, I haven’t seen any relevant FreeBSD failures on CI for at least a month, so if it’s not fixed, the chances of debugging are very low. Signed-off-by: Philip Withnall https://gitlab.gnome.org/GNOME/glib/issues/1614 --- gio/tests/meson.build | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gio/tests/meson.build b/gio/tests/meson.build index a3efd33ab..e774721b4 100644 --- a/gio/tests/meson.build +++ b/gio/tests/meson.build @@ -248,10 +248,10 @@ if host_machine.system() != 'windows' 'extra_sources' : extra_sources, 'suite' : ['slow'], }, - 'gdbus-auth' : {'extra_sources' : extra_sources, 'suite': ['flaky']}, - 'gdbus-bz627724' : {'extra_sources' : extra_sources, 'suite': ['flaky']}, + 'gdbus-auth' : {'extra_sources' : extra_sources}, + 'gdbus-bz627724' : {'extra_sources' : extra_sources}, 'gdbus-close-pending' : {'extra_sources' : extra_sources}, - 'gdbus-connection' : {'extra_sources' : extra_sources, 'suite': ['flaky']}, + 'gdbus-connection' : {'extra_sources' : extra_sources}, 'gdbus-connection-loss' : {'extra_sources' : extra_sources}, 'gdbus-connection-slow' : {'extra_sources' : extra_sources}, 'gdbus-error' : {'extra_sources' : extra_sources},