[Spice-devel] [spice-gtk v1 1/2] channel-webdav: implement channel-reset

[Marc-André Lureau]

----- 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_unref() in this patch.
> 
> 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-webdav.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> index 4a246b5..a4e2215 100644
> --- a/src/channel-webdav.c
> +++ b/src/channel-webdav.c
> @@ -563,7 +563,29 @@ 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) {
> +        /* In case the channel has been reset */
> +        spice_webdav_channel_init(SPICE_WEBDAV_CHANNEL(channel));
> +    }
> +}
> +
> +static void spice_webdav_channel_reset(SpiceChannel *channel, gboolean
> migrating)
> +{
> +    SpiceWebdavChannel *self = SPICE_WEBDAV_CHANNEL(channel);
> +    SpiceWebdavChannelPrivate *c = self->priv;
> +
> +    c->demuxing = FALSE;
> +    g_cancellable_cancel(c->cancellable);
> +    g_clear_object(&c->cancellable);
> +    g_hash_table_unref(c->clients);

hmm, it's only created at _init() time.

I think it would be better to call port_set_opened(self, false) in spice_port_channel_reset(), this would make this code redundant, and it would be quite logical.

However, I am not clear about migration handling, if it was tested, it was a long time ago...

We may want to keep some state if migrating=true..

> +    g_clear_pointer(&c->queue, output_queue_free);
> +    g_clear_object(&c->stream);

Same problem it's created only at _init() time

nack for now, it needs further work

> +
> +
> SPICE_CHANNEL_CLASS(spice_webdav_channel_parent_class)->channel_reset(channel,
> migrating);
>  }
>  
>  static void spice_webdav_channel_class_init(SpiceWebdavChannelClass *klass)
> @@ -575,6 +597,7 @@ static void
> spice_webdav_channel_class_init(SpiceWebdavChannelClass *klass)
>      gobject_class->finalize     = spice_webdav_channel_finalize;
>      channel_class->handle_msg   = spice_webdav_handle_msg;
>      channel_class->channel_up   = spice_webdav_channel_up;
> +    channel_class->channel_reset = spice_webdav_channel_reset;
>  
>      g_signal_override_class_handler("port-event",
>                                      SPICE_TYPE_WEBDAV_CHANNEL,
> --
> 2.13.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>