[Spice-devel] [PATCH spice-gtk 02/15] channel-main: Fix dangling references to freed file-xfer-tasks on agent cancel

[Hans de Goede]

While testing the agent error handling code I was triggering the
agent-file-xfer-cancel code-path in spice-gtk. This crashes due to the
flushing queue still having a reference to the task in question when its
gets cancelled from the agent side.

My previous fix for this added code to file_xfer_close_cb, to remove any
the task being closed from the flushing queue. Although this fixes my
test-case for this, on further thought this fix is not good enough.

The problem is that file_xfer_flushed / file_xfer_flush_async execute
file_xfer_data_flushed_cb from an idle handler, so it is possible that when
file_xfer_close_cb runs and frees the task, it is not part of the flushing
queue, but a file_xfer_data_flushed_cb with the task as user_data argument
still needs to run, and when it will run it will refer to the now freed task.

This new version of this patch fixes this by:
1) Passing the task id, rather then a pointer to file_xfer_data_flushed_cb
2) Let file_xfer_data_flushed_cb look up the id
3) If the id is not found exit without logging an error

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 gtk/channel-main.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/gtk/channel-main.c b/gtk/channel-main.c
index a1e1e1d..3a8229f 100644
--- a/gtk/channel-main.c
+++ b/gtk/channel-main.c
@@ -1558,12 +1558,26 @@ static void file_xfer_data_flushed_cb(GObject *source_object,
                                       GAsyncResult *res,
                                       gpointer user_data)
 {
-    SpiceFileXferTask *task = user_data;
+    SpiceFileXferTask *task;
     SpiceMainChannel *channel = (SpiceMainChannel *)source_object;
     GError *error = NULL;
+    uint32_t id = GPOINTER_TO_UINT(user_data);
+    GList *l;
+
+    /* We must do this before file_xfer_flush_finish(), because
+       file_xfer_flush_finish() may free the last channel reference */
+    l = g_list_find_custom(channel->priv->file_xfer_task_list, &id,
+                           file_xfer_task_find);
 
     file_xfer_flush_finish(channel, res, &error);
 
+    if (l == NULL) {
+        g_clear_error(&error);
+        return;
+    }
+
+    task = l->data;
+
     if (error != NULL) {
         g_warning("failed to flush xfer queue: %s", error->message);
         task->error = error;
@@ -1612,7 +1626,8 @@ static void file_xfer_read_cb(GObject *source_object,
         task->read_bytes += count;
         file_xfer_queue(task, count);
         file_xfer_flush_async(channel, task->cancellable,
-                              file_xfer_data_flushed_cb, task);
+                              file_xfer_data_flushed_cb,
+                              GUINT_TO_POINTER(task->id));
     } else if (error) {
         VDAgentFileXferStatusMessage msg = {
             .id = task->id,
-- 
1.8.1.4