[Spice-devel] [spice-gtk v2 8/9] channel-main: avoid race around file-transfer flush

[Victor Toso]

This patch avoids a race condition. The race happens when we mark the
xfer-task as completed by receiving VD_AGENT_FILE_XFER_STATUS_SUCCESS
message while the flush callback was not yet called.

The flush callback is file_xfer_data_flushed_cb() and it might not be
called immediately after data is flushed.

The race can be verified while transferring several small files at
once. I can see it often with more then 50 files in one transfer
operation.

This fix implies that SpiceMainChannel should check in its async
callbacks if given SpiceFileTransferTask is completed.

This patch introduces spice_file_transfer_task_is_completed (internal)
to help check if spice_file_transfer_task_completed() was called or
not.
---
 src/channel-main.c                  | 17 +++++++++++------
 src/spice-file-transfer-task-priv.h |  1 +
 src/spice-file-transfer-task.c      | 14 ++++++++++++++
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/src/channel-main.c b/src/channel-main.c
index bc7349f..14ad6cf 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1766,10 +1766,12 @@ static void file_xfer_data_flushed_cb(GObject *source_object,
         return;
     }
 
-    file_transfer_operation_send_progress(xfer_task);
-
-    /* Read more data */
-    spice_file_transfer_task_read_async(xfer_task, file_xfer_read_async_cb, NULL);
+    /* task might be completed while on idle */
+    if (!spice_file_transfer_task_is_completed(xfer_task)) {
+        file_transfer_operation_send_progress(xfer_task);
+        /* Read more data */
+        spice_file_transfer_task_read_async(xfer_task, file_xfer_read_async_cb, NULL);
+    }
 }
 
 static void file_xfer_queue_msg_to_agent(SpiceMainChannel *channel,
@@ -1814,13 +1816,15 @@ static void file_xfer_read_async_cb(GObject *source_object,
     }
 
     file_xfer_queue_msg_to_agent(channel, spice_file_transfer_task_get_id(xfer_task), buffer, count);
-    if (count == 0) {
-        /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent */
+    if (count == 0 || spice_file_transfer_task_is_completed(xfer_task)) {
+        /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent
+         * in case the task was completed, nothing to do. */
         return;
     }
 
     task_id = spice_file_transfer_task_get_id(xfer_task);
     xfer_op = g_hash_table_lookup(channel->priv->file_xfer_tasks, GUINT_TO_POINTER(task_id));
+    g_return_if_fail(xfer_op != NULL);
     xfer_op->stats.total_sent += count;
 
     cancellable = spice_file_transfer_task_get_cancellable(xfer_task);
@@ -1841,6 +1845,7 @@ static void main_agent_handle_xfer_status(SpiceMainChannel *channel,
 
     switch (msg->result) {
     case VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA:
+        g_return_if_fail(spice_file_transfer_task_is_completed(xfer_task) == FALSE);
         spice_file_transfer_task_read_async(xfer_task, file_xfer_read_async_cb, NULL);
         return;
     case VD_AGENT_FILE_XFER_STATUS_CANCELLED:
diff --git a/src/spice-file-transfer-task-priv.h b/src/spice-file-transfer-task-priv.h
index 1ceb392..a7b58d8 100644
--- a/src/spice-file-transfer-task-priv.h
+++ b/src/spice-file-transfer-task-priv.h
@@ -52,6 +52,7 @@ gssize spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
                                             GError **error);
 guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask *self);
 guint64 spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask *self);
+gboolean spice_file_transfer_task_is_completed(SpiceFileTransferTask *self);
 
 G_END_DECLS
 
diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
index eb943c4..4e51b7e 100644
--- a/src/spice-file-transfer-task.c
+++ b/src/spice-file-transfer-task.c
@@ -42,6 +42,7 @@ struct _SpiceFileTransferTask
     GObject parent;
 
     uint32_t                       id;
+    gboolean                       completed;
     gboolean                       pending;
     GFile                          *file;
     SpiceMainChannel               *channel;
@@ -270,6 +271,8 @@ G_GNUC_INTERNAL
 void spice_file_transfer_task_completed(SpiceFileTransferTask *self,
                                         GError *error)
 {
+    self->completed = TRUE;
+
     /* In case of multiple errors we only report the first error */
     if (self->error)
         g_clear_error(&error);
@@ -478,6 +481,16 @@ gssize spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
     return nbytes;
 }
 
+G_GNUC_INTERNAL
+gboolean spice_file_transfer_task_is_completed(SpiceFileTransferTask *self)
+{
+    g_return_val_if_fail(self != NULL, FALSE);
+
+    /* File transfer is considered over at the first time it is marked as
+     * complete with spice_file_transfer_task_completed. */
+    return self->completed;
+}
+
 /*******************************************************************************
  * External API
  ******************************************************************************/
@@ -735,4 +748,5 @@ static void
 spice_file_transfer_task_init(SpiceFileTransferTask *self)
 {
     self->buffer = g_malloc0(FILE_XFER_CHUNK_SIZE);
+    self->completed = FALSE;
 }
-- 
2.7.4