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

[Jonathon Jongsma]

On Thu, 2015-11-05 at 16:36 -0500, Marc-André Lureau wrote:
> 
> ----- 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.


There's still the unref that was originally freeing the task: in
file_xfer_close_cb()


> 
> >          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
> >