[Spice-devel] [PATCH spice-gtk] file xfer: Fix segfault when rebooting

[Jonathon Jongsma]

Recent changes to file transfer introduced a regression where the client
would crash when rebooting a guest after performing a file transfer.
This was caused because the SpiceFileTransferTask is freed when it is
completed, but is not removed from the MainChannel's hash table. When we
reboot the guest and lose our vdagent connection, we iterate through the
list of tasks in the hash table and complete them. But since we did not
remove the already-completed tasks from this hash table, this hash table
contains already-freed memory.

To fix the issue, take an extra ref for the async operations (so that
completing the task won't free an object that is stored in the hash table). In
addition, connect to the task's "finished" signal and remove it from the hash
table when it becomes finished.

Bug reported via email by Jay.han <ezzzehxx at gmail.com>. Valgrind report
below:

==6926== Invalid read of size 8
==6926==    at 0x508177B: spice_file_transfer_task_completed (channel-main.c:2941)
==6926==    by 0x50846DC: set_agent_connected (channel-main.c:462)
==6926==    by 0x5073A43: spice_channel_recv_msg (spice-channel.c:1892)
==6926==    by 0x5073BE3: spice_channel_iterate_read (spice-channel.c:2132)
==6926==    by 0x5075D25: spice_channel_coroutine (spice-channel.c:2170)
==6926==    by 0x50A6EFE: coroutine_trampoline (coroutine_ucontext.c:63)
==6926==    by 0x50A6CC8: continuation_trampoline (continuation.c:55)
==6926==    by 0x65C2B5F: ??? (in /lib/x86_64-linux-gnu/libc-2.19.so)
==6926==    by 0x151331C7: ???
==6926==  Address 0x29971fd8 is 168 bytes inside a block of size 184 free'd
==6926==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6926==    by 0x5E33142: g_type_free_instance (in /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0.4000.0)
==6926==    by 0x50815DA: file_xfer_close_cb (channel-main.c:1826)
==6926==    by 0x5AEBD5C: ??? (in /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.4000.0)
==6926==    by 0x5B0F41A: ??? (in /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.4000.0)
==6926==    by 0x5B0F438: ??? (in /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.4000.0)
==6926==    by 0x609BCE4: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
==6926==    by 0x609C047: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
==6926==    by 0x609C309: g_main_loop_run (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
==6926==    by 0x4058AB: main (spicy.c:1858)
---
 src/channel-main.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/channel-main.c b/src/channel-main.c
index 0eb40b9..5abb32d 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -3041,6 +3041,16 @@ static SpiceFileTransferTask *spice_file_transfer_task_new(SpiceMainChannel *cha
                                                            GFile *file,
                                                            GCancellable *cancellable);
 
+static void task_finished(SpiceFileTransferTask *task,
+                          GError *error,
+                          gpointer data)
+{
+    SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data);
+    g_debug("%s: %i", G_STRFUNC, task->priv->id);
+
+    g_hash_table_remove(channel->priv->file_xfer_tasks, GUINT_TO_POINTER(task->priv->id));
+}
+
 static void file_xfer_send_start_msg_async(SpiceMainChannel *channel,
                                            GFile **files,
                                            GFileCopyFlags flags,
@@ -3074,13 +3084,14 @@ static void file_xfer_send_start_msg_async(SpiceMainChannel *channel,
         g_hash_table_insert(c->file_xfer_tasks,
                             GUINT_TO_POINTER(task->priv->id),
                             task);
+        g_signal_connect(task, "finished", G_CALLBACK(task_finished), channel);
         g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0, task);
 
         g_file_read_async(files[i],
                           G_PRIORITY_DEFAULT,
                           cancellable,
                           file_xfer_read_async_cb,
-                          task);
+                          g_object_ref(task));
         task->priv->pending = TRUE;
 
         /* if we created a per-task cancellable above, free it */
-- 
2.4.3