[Spice-devel] [spice-gtk v1 1/2] file-xfer: fix segfault on agent disconnection

[Victor Toso]

Hi,

On Thu, May 12, 2016 at 10:51:02PM +0200, Fabiano Fidêncio wrote:
> On Thu, May 12, 2016 at 10:42 PM, Victor Toso <victortoso at redhat.com> wrote:
> > As one can see by the backtrace, the reason for the segfault is that
> > g_task_return_now() is called under coroutine context and in
> > spice_file_transfer_task_completed() we access memory that the
> > coroutine context has no access.
> >
> > With GTask integration its callback must respect the coroutine context
> > or return-in-idle so the callback can be called in the main context.
> >
> > In this specific case the memory access can be fixed also due the
> > error that is present which is agent being disconnected during a
> > file-xfer. In that situation, we should not try to send a message to
> > the agent.
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > spice_file_transfer_task_completed (self=self at entry=0x7fffd0006f00, error=0x0) at channel-main.c:2963
> > 2963            VDAgentFileXferStatusMessage msg = {
> > (gdb) bt
> >  #0  spice_file_transfer_task_completed (self=self at entry=0x7fffd0006f00, error=0x0) at channel-main.c:2963
> >  #1  in file_xfer_data_flushed_cb (source_object=0x7cc1d0, res=0x953390, user_data=user_data at entry=0x7fffd0006f00) at channel-main.c:1857
> >  #2  in g_task_return_now (task=0x953390) at gtask.c:1108
> >  #3  in g_task_return (task=0x953390, type=<optimized out>) at gtask.c:1166
> >  #4  in flush_foreach_remove (key=<optimized out>, value=<optimized out>, user_data=<optimized out>) at channel-main.c:928
> >  #5  in g_hash_table_foreach_remove_or_steal (hash_table=0x70cea0, func=func at entry=0x7ffff5616f10 <flush_foreach_remove>, user_data=user_data at entry=0x0, notify=notify at entry=1) at ghash.c:1492
> >  #6  in g_hash_table_foreach_remove (hash_table=<optimized out>, func=func at entry=0x7ffff5616f10 <flush_foreach_remove>, user_data=user_data at entry=0x0) at ghash.c:1538
> >  #7  in file_xfer_flushed (success=0, channel=0x7cc1d0) at channel-main.c:936
> >  #8 _reset_agent (channel=0x7cc1d0) at channel-main.c:466
> >  #9 d (channel=0x7cc1d0, connected=connected at entry=0) at channel-main.c:1572
> >  #10 in spice_main_channel_reset (channel=0x7cc1d0, migrating=0) at channel-main.c:485
> >  #11 in spice_channel_coroutine (data=0x7cc1d0) at spice-channel.c:2564
> >  #12 in coroutine_trampoline (cc=0x7cb860) at coroutine_ucontext.c:63
> >  #13 in continuation_trampoline (i0=<optimized out>, i1=<optimized out>) at continuation.c:55
> >  #14 in ?? () from /lib64/libc.so.6
> >  #15 in ?? ()
> >  #16 in ?? ()
> > Backtrace stopped: Cannot access memory at address
> > ---
> >  src/channel-main.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index f7789dd..fb0630e 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -2950,6 +2950,8 @@ void spice_main_set_display_enabled(SpiceMainChannel *channel, int id, gboolean
> >  static void spice_file_transfer_task_completed(SpiceFileTransferTask *self,
> >                                                 GError *error)
> >  {
> > +    SpiceMainChannelPrivate *main_priv = self->priv->channel->priv;
> 
> Ouch, quite ugly :-\.
> IMO would be better to introduce a
> spice_main_channel_get_agent_connected() function or something like
> that.
> Do you think it would make sense?.

I do! I'll send a new one tomorrow.

Cheers,
  toso

>
> > +
> >      /* In case of multiple errors we only report the first error */
> >      if (self->priv->error)
> >          g_clear_error(&error);
> > @@ -2959,7 +2961,7 @@ static void spice_file_transfer_task_completed(SpiceFileTransferTask *self,
> >          self->priv->error = error;
> >      }
> >
> > -    if (self->priv->error) {
> > +    if (self->priv->error && main_priv->agent_connected) {
> >          VDAgentFileXferStatusMessage msg = {
> >              .id = self->priv->id,
> >              .result = error->code == G_IO_ERROR_CANCELLED ?
> > --
> > 2.5.5
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> Reviewed-by: Fabiano Fidêncio <fidencio at redhat.com>
> -- 
> Fabiano Fidêncio