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

[Marc-André Lureau]

----- Original Message -----
> 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));

This looks like it will leak. There is now 2 refs, right? where are the corresponding 2 unrefs? I count one with the hash table.

>          task->priv->pending = TRUE;
>  
>          /* if we created a per-task cancellable above, free it */
> --
> 2.4.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>