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@xxxxxxxxx>. 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel