[Spice-devel] [PATCH spice-gtk 2/2] channel: Abort migration in delayed unref

[Victor Toso]

Hi,

On Tue, Apr 26, 2016 at 01:17:58PM +0200, Pavel Grunt wrote:
> Hi,
> 
> On Tue, 2016-04-26 at 12:05 +0200, Victor Toso wrote:
> > Hi,
> > 
> > On Fri, Apr 22, 2016 at 04:47:48PM +0200, Pavel Grunt wrote:
> > > When channel is unref'ed during migration migrate_channel_event_cb
> > > is called causing a crash by coroutine yielding to nonexistent channel.
> > > 
> > > As comment in spice_channel_coroutine says:
> > >   Co-routine exits now - the SpiceChannel object may no longer exist,
> > >   so don't do anything else now unless you like SEGVs
> > > 
> > > Related: rhbz#1318574
> > > ---
> > >  src/spice-channel.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/src/spice-channel.c b/src/spice-channel.c
> > > index 19237b3..7b0a3dc 100644
> > > --- a/src/spice-channel.c
> > > +++ b/src/spice-channel.c
> > > @@ -2296,6 +2296,7 @@ static gboolean spice_channel_delayed_unref(gpointer
> > > data)
> > >      SpiceChannel *channel = SPICE_CHANNEL(data);
> > >      SpiceChannelPrivate *c = channel->priv;
> > >      gboolean was_ready = c->state == SPICE_CHANNEL_STATE_READY;
> > > +    SpiceSession *session;
> > >  
> > >      CHANNEL_DEBUG(channel, "Delayed unref channel %p", channel);
> > >  
> > > @@ -2303,6 +2304,13 @@ static gboolean spice_channel_delayed_unref(gpointer
> > > data)
> > >  
> > >      c->state = SPICE_CHANNEL_STATE_UNCONNECTED;
> > >  
> > > +    session = spice_channel_get_session(channel);
> > > +    if (spice_session_is_for_migration(session)) {
> > > +        /* error during migration - abort migration */
> > > +        spice_session_abort_migration(session);
> > > +        return FALSE;
> > > +    }
> > > +
> >
> > Just to be sure if I got it right. The migration fails for some reason which
> > triggers and error in the migration process for spice-gtk. That will make the
> > channel for the target host to be delayed_unref.
> >
> > If we don't spice_session_abort_migration() now, migrate_channel_event_cb()
> > will
> > be called for the channel for the former host which will try to yield to a
> > coroutine and cause the crash.
> yes
> >
> > Does this only happen under failure in the migration like [0] or it could
> > happen
> > if we do cancel the migration in the right moment?
>
> if migration is canceled by the SPICE_MSG_MAIN_MIGRATE_CANCEL
> message, spice_session_abort_migration() is called.
>
> Looking at the backtrace of [0] the patch addresses the same issue.
>
> Pavel

Sorry for taking some time to test it. I've just tested the patch and
remote-viewer did not crash on the situation around [0].

I'd suggest just to improve the commit message to point out that the
delayed_unref happens for the target host channel and will only occur
when the migration process fails.

Acked-by: Victor Toso <victortoso at redhat.com>

PS: feel free to close fdo#92019 after pushing this!

>
> > 
> > [0] https://bugs.freedesktop.org/show_bug.cgi?id=92019
> > 
> > Cheers,
> >   toso
> > 
> > >      if (c->event != SPICE_CHANNEL_NONE) {
> > >          g_coroutine_signal_emit(channel, signals[SPICE_CHANNEL_EVENT], 0,
> > > c->event);
> > >          c->event = SPICE_CHANNEL_NONE;