From c033450f9369a50c91fa957da280c02c050093d6 Mon Sep 17 00:00:00 2001 From: Carlos Garnacho Date: Mon, 24 Aug 2020 12:00:44 +0200 Subject: [PATCH 1/2] goutputstream: Check individual close operations after splice After a splice operation is finished, it attempts to 1) close input/output streams, as per the given flags, and 2) return the operation result (maybe an error, too). However, if the operation gets cancelled early and the streams indirectly closed, the splice operation will try to close both descriptors and return on the task when both are already closed. The catch here is that getting the streams closed under its feet is possible, so the completion callback would find both streams closed after returning on the first close operation and return the error, but then the second operation could be able to trigger a second error which would be returned as well. What happens here is up to further race conditions, if the task didn't return yet, the returned error will be simply replaced (but the old one not freed...), if it did already return, it'll result in: GLib-GIO-FATAL-CRITICAL: g_task_return_error: assertion '!task->ever_returned' failed Fix this by flagging the close_async() callbacks, and checking that both close operations did return, instead of checking that both streams are closed by who knows. This error triggers a semi-frequent CI failure in tracker, see the summary at https://gitlab.gnome.org/GNOME/tracker/-/issues/240 --- gio/goutputstream.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/gio/goutputstream.c b/gio/goutputstream.c index 87b61a4ec..9e2848e37 100644 --- a/gio/goutputstream.c +++ b/gio/goutputstream.c @@ -2643,6 +2643,8 @@ g_output_stream_real_writev_finish (GOutputStream *stream, typedef struct { GInputStream *source; GOutputStreamSpliceFlags flags; + guint istream_closed : 1; + guint ostream_closed : 1; gssize n_read; gssize n_written; gsize bytes_copied; @@ -2665,11 +2667,11 @@ real_splice_async_complete_cb (GTask *task) SpliceData *op = g_task_get_task_data (task); if (op->flags & G_OUTPUT_STREAM_SPLICE_CLOSE_SOURCE && - !g_input_stream_is_closed (op->source)) + !op->istream_closed) return; if (op->flags & G_OUTPUT_STREAM_SPLICE_CLOSE_TARGET && - !g_output_stream_is_closed (g_task_get_source_object (task))) + !op->ostream_closed) return; if (op->error != NULL) @@ -2691,8 +2693,10 @@ real_splice_async_close_input_cb (GObject *source, gpointer user_data) { GTask *task = user_data; + SpliceData *op = g_task_get_task_data (task); g_input_stream_close_finish (G_INPUT_STREAM (source), res, NULL); + op->istream_closed = TRUE; real_splice_async_complete_cb (task); } @@ -2707,6 +2711,7 @@ real_splice_async_close_output_cb (GObject *source, GError **error = (op->error == NULL) ? &op->error : NULL; g_output_stream_internal_close_finish (G_OUTPUT_STREAM (source), res, error); + op->ostream_closed = TRUE; real_splice_async_complete_cb (task); } From cf85241abaa07bbf5db372a39fe0d320f0d5093e Mon Sep 17 00:00:00 2001 From: Carlos Garnacho Date: Mon, 24 Aug 2020 12:30:16 +0200 Subject: [PATCH 2/2] tests: Add splice cancellation test This doesn't trigger the cancellation assertion issue when run locally (the task didn't return yet, so the error is simply overwritten), but perhaps it ever does in CI. Anyhow, it's good to have a cancellation test. --- gio/tests/async-splice-output-stream.c | 27 +++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/gio/tests/async-splice-output-stream.c b/gio/tests/async-splice-output-stream.c index e69c4db5d..fb317b733 100644 --- a/gio/tests/async-splice-output-stream.c +++ b/gio/tests/async-splice-output-stream.c @@ -32,6 +32,7 @@ typedef enum TEST_THREADED_NONE = 0, TEST_THREADED_ISTREAM = 1, TEST_THREADED_OSTREAM = 2, + TEST_CANCEL = 4, TEST_THREADED_BOTH = TEST_THREADED_ISTREAM | TEST_THREADED_OSTREAM, } TestThreadedFlags; @@ -58,6 +59,14 @@ test_copy_chunks_splice_cb (GObject *source, bytes_spliced = g_output_stream_splice_finish (G_OUTPUT_STREAM (source), res, &error); + + if (data->flags & TEST_CANCEL) + { + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_CANCELLED); + g_main_loop_quit (data->main_loop); + return; + } + g_assert_no_error (error); g_assert_cmpint (bytes_spliced, ==, strlen (data->data)); @@ -100,11 +109,18 @@ test_copy_chunks_start (TestThreadedFlags flags) { TestCopyChunksData data; GError *error = NULL; + GCancellable *cancellable = NULL; data.main_loop = g_main_loop_new (NULL, FALSE); data.data = "abcdefghijklmnopqrstuvwxyz"; data.flags = flags; + if (data.flags & TEST_CANCEL) + { + cancellable = g_cancellable_new (); + g_cancellable_cancel (cancellable); + } + if (data.flags & TEST_THREADED_ISTREAM) { GFile *file; @@ -150,7 +166,7 @@ test_copy_chunks_start (TestThreadedFlags flags) g_output_stream_splice_async (data.ostream, data.istream, G_OUTPUT_STREAM_SPLICE_CLOSE_SOURCE | G_OUTPUT_STREAM_SPLICE_CLOSE_TARGET, - G_PRIORITY_DEFAULT, NULL, + G_PRIORITY_DEFAULT, cancellable, test_copy_chunks_splice_cb, &data); /* We do not hold a ref in data struct, this is to make sure the operation @@ -158,6 +174,7 @@ test_copy_chunks_start (TestThreadedFlags flags) */ g_object_unref (data.istream); g_object_unref (data.ostream); + g_clear_object (&cancellable); g_main_loop_run (data.main_loop); g_main_loop_unref (data.main_loop); @@ -187,6 +204,12 @@ test_copy_chunks_threaded (void) test_copy_chunks_start (TEST_THREADED_BOTH); } +static void +test_cancelled (void) +{ + test_copy_chunks_start (TEST_THREADED_NONE | TEST_CANCEL); +} + int main (int argc, char *argv[]) @@ -200,6 +223,8 @@ main (int argc, test_copy_chunks_threaded_output); g_test_add_func ("/async-splice/copy-chunks-threaded", test_copy_chunks_threaded); + g_test_add_func ("/async-splice/cancelled", + test_cancelled); return g_test_run(); }