[Spice-devel] [spice-gtk v2 1/2] channel-port: set port closed on channel-reset

[Marc-André Lureau]

Hi

----- Original Message -----
> From: Victor Toso <me at victortoso.com>
> 
> The channel can be reset after disabling the shared-folder. If we had
> pending read, we should cancel it using the GCancellable for the
> demuxing code on vmcstream.c (c->cancellable) and per client operation
> (client->cancellable) which is done on client_remove_unref() called by
> g_hash_table_remove_all() in port_event()
> 
> This bug resolves rhbz#1474074 for Linux guests, tested on Fedora 25.
> 
> Reproducer:
>  1. With remote-viewer, connect to a Fedora 25 (GNOME) with
>     spice-webavd running;
>  2. In remote-viewer, enable shared-folder (File > Preferences);
>  3. In the guest, open Nautilus, go to "Other Locations" and double
>     click at "Spice client folder" to mount that webdav folder;
>  4. Wait for the folder to be mounted in the guest;
>  5. In remote-viewer, disabled shared-folder;
>  6. In the guest, try to access the mounted folder. It should fail and
>     the mount point will be removed;
>  7. Repeat steps 2->3 and see the client crash.
> 
> > Thread 1 "remote-viewer" received signal SIGSEGV, Segmentation fault.
> > (gdb) bt full
> > #0  in __memmove_avx_unaligned_erms () from /lib64/libc.so.6
> > #1  in spice_channel_read_sasl (channel=0xca8130, data=0xfc2050, len=6) at
> > spice-channel.c:1122
> > #2  in spice_channel_read (channel=0xca8130, data=0xfc2050, length=6) at
> > spice-channel.c:1149
> > #3  in spice_channel_recv_msg (channel=0xca8130,
>       msg_handler=0x7ffff4e52e3c <spice_webdav_handle_msg>, data=0x0) at
>       spice-channel.c:2031
> > #4  in spice_channel_iterate_read (channel=0xca8130) at
> > spice-channel.c:2338
> > #5  in spice_channel_iterate (channel=0xca8130) at spice-channel.c:2376
> > #6  in spice_channel_coroutine (data=0xca8130) at spice-channel.c:2664
> > #7  0x00007ffff4e92d49 in coroutine_trampoline (cc=0xca77e0) at
> > coroutine_ucontext.c:63
> > #8  0x00007ffff4e92a01 in continuation_trampoline (i0=13268960, i1=0) at
> > continuation.c:55
> > #9  0x00007fffefce7600 in ?? () from /lib64/libc.so.6
> 
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1474074
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/channel-port.c   |  7 +++++--
>  src/channel-webdav.c | 13 +++++++++++++
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/src/channel-port.c b/src/channel-port.c
> index d922e4b..f9d12e8 100644
> --- a/src/channel-port.c
> +++ b/src/channel-port.c
> @@ -22,6 +22,8 @@
>  #include "spice-channel-priv.h"
>  #include "spice-marshal.h"
>  
> +static void port_set_opened(SpicePortChannel *self, gboolean opened);
> +
>  /**
>   * SECTION:channel-port
>   * @short_description: private communication channel
> @@ -116,10 +118,11 @@ static void spice_port_channel_finalize(GObject
> *object)
>  
>  static void spice_port_channel_reset(SpiceChannel *channel, gboolean
>  migrating)
>  {
> -    SpicePortChannelPrivate *c = SPICE_PORT_CHANNEL(channel)->priv;
> +    SpicePortChannel *self = SPICE_PORT_CHANNEL(channel);
> +    SpicePortChannelPrivate *c = self->priv;
>  
>      g_clear_pointer(&c->name, g_free);
> -    c->opened = FALSE;
> +    port_set_opened(self, false);
>  

So regarding migration, channel_reset is only used for SEMI_SEAMLESS, which can't guarantee the disruption of the stream by design (see https://cgit.freedesktop.org/spice/spice-protocol/commit/?id=3838ad140a046c4ddf42fef58c9727ecfdc09f9f for details). So it seems fine to reopen the port here.

>      SPICE_CHANNEL_CLASS(spice_port_channel_parent_class)->channel_reset(channel,
>      migrating);
>  }
> diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> index 4a246b5..a38fa19 100644
> --- a/src/channel-webdav.c
> +++ b/src/channel-webdav.c
> @@ -514,6 +514,8 @@ static void port_event(SpiceWebdavChannel *self, gint
> event)
>          g_cancellable_cancel(c->cancellable);
>          c->demuxing = FALSE;
>          g_hash_table_remove_all(c->clients);
> +        g_clear_pointer(&c->queue, output_queue_free);
> +        g_clear_object(&c->stream);
>      }
>  }
>  
> @@ -563,7 +565,18 @@ static void spice_webdav_channel_dispose(GObject
> *object)
>  
>  static void spice_webdav_channel_up(SpiceChannel *channel)
>  {
> +    SpiceWebdavChannelPrivate *c = SPICE_WEBDAV_CHANNEL(channel)->priv;
> +
>      CHANNEL_DEBUG(channel, "up");
> +
> +    if (c->stream == NULL) {
> +        GOutputStream *ostream;
> +
> +        /* In case the channel has been reset */
> +        c->stream = spice_vmc_stream_new(SPICE_CHANNEL(channel));
> +        ostream = g_io_stream_get_output_stream(G_IO_STREAM(c->stream));
> +        c->queue = output_queue_new(ostream);

Please drop the same code from init.

> +    }
>  }
>  

With that, the patch looks correct to me, and testing is ok.

I can still use the webdav channel after seamless migration, and doing a copy in a loop didn't show any error (more testing would be needed to check for corruption).


ack