Hi, Thanks for the quick review! On Tue, Aug 01, 2017 at 09:37:04AM -0400, Marc-André Lureau wrote: > > > ----- Original Message ----- > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > 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. Hm, that did not work. The reason is that channel_reset() from spice-channel seems to be called before spice_webdav_channel_reset() and spice_port_channel_reset(). At the time we do port_set_opened(self, false), the event does not reach port_event() from channel-webdav.c Well, that happens only if I implement channel_reset() in channel-webdav.c. If I move the two changes below to port_event() and remove channel_reset(), it works. Do you know why? > 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.. Yes, I've tested on Windows 7 which is the one I expect issues ;) With and without this patch, the mount point is not lost but I agree that the change below should affect any ongoing operation and likely will break the communication... > > + 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 I'll be sending a new patch soon! I think the correct behavior is implementing channel-reset in channel-webdav and handling correctly the case migrating==true but I don't really think that's a hard requirement for now. Cheers, toso > > + > > + > > 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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > >
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel