From 052ad6098ded95418704a27f487adf7bf97e14b1 Mon Sep 17 00:00:00 2001 From: Stefan Sperling Date: Sat, 23 Mar 2019 12:07:47 +0100 Subject: [PATCH] Fix use-after-free triggered by gnome-session-binary ostream_flush_cb() was calling flush_data_list_complete() with a single element list with an item that had already been freed. This was observed on OpenBSD where memory is overwritten with 0xdf during free(): error=0x0) at ../glib-2.58.3/gio/gdbusprivate.c:1156 1156 g_mutex_lock (&f->mutex); (gdb) p /x *f $74 = {mutex = {p = 0xdfdfdfdfdfdfdfdf, i = {0xdfdfdfdf, 0xdfdfdfdf}}, cond = { p = 0xdfdfdfdfdfdfdfdf, i = {0xdfdfdfdf, 0xdfdfdfdf}}, number_to_wait_for = 0xdfdfdfdfdfdfdfdf, error = 0x0} This happened because the thread freeing the element didn't properly wait for the asynchronous flush operation to finish. Gnome's developer docs say: "g_cond_wait() must always be used in a loop" https://developer.gnome.org/glib/stable/glib-Threads.html#g-cond-wait --- gio/gdbusprivate.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c index 4d7ca6f89..dee1f2719 100644 --- a/gio/gdbusprivate.c +++ b/gio/gdbusprivate.c @@ -1194,13 +1194,6 @@ ostream_flush_cb (GObject *source_object, } } - g_assert (data->flushers != NULL); - flush_data_list_complete (data->flushers, error); - g_list_free (data->flushers); - - if (error != NULL) - g_error_free (error); - /* Make sure we tell folks that we don't have additional flushes pending */ g_mutex_lock (&data->worker->write_lock); @@ -1209,6 +1202,12 @@ ostream_flush_cb (GObject *source_object, data->worker->output_pending = PENDING_NONE; g_mutex_unlock (&data->worker->write_lock); + g_assert (data->flushers != NULL); + flush_data_list_complete (data->flushers, error); + g_list_free (data->flushers); + if (error != NULL) + g_error_free (error); + /* OK, cool, finally kick off the next write */ continue_writing (data->worker); @@ -1377,6 +1376,10 @@ iostream_close_cb (GObject *source_object, g_assert (worker->output_pending == PENDING_CLOSE); worker->output_pending = PENDING_NONE; + /* Ensure threads waiting for pending flushes to finish will be unblocked. */ + worker->write_num_messages_flushed = + worker->write_num_messages_written + g_list_length(pending_flush_attempts); + g_mutex_unlock (&worker->write_lock); while (pending_close_attempts != NULL) @@ -1792,10 +1795,17 @@ _g_dbus_worker_flush_sync (GDBusWorker *worker, if (data != NULL) { - g_cond_wait (&data->cond, &data->mutex); - g_mutex_unlock (&data->mutex); + /* Wait for flush operations to finish. */ + g_mutex_lock (&worker->write_lock); + while (worker->write_num_messages_flushed < data->number_to_wait_for) + { + g_mutex_unlock (&worker->write_lock); + g_cond_wait (&data->cond, &data->mutex); + g_mutex_lock (&worker->write_lock); + } + g_mutex_unlock (&worker->write_lock); - /* note:the element is removed from worker->write_pending_flushes in flush_cb() above */ + g_mutex_unlock (&data->mutex); g_cond_clear (&data->cond); g_mutex_clear (&data->mutex); if (data->error != NULL)