Hi, On Wed, Jan 16, 2019 at 12:40:36PM +0400, Marc-André Lureau wrote: > Hi > > On Wed, Jan 16, 2019 at 11:44 AM Victor Toso <victortoso@xxxxxxxxxx> wrote: > > > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > Similar to 172c521271a3d - if we don't, the agent might be waiting for > > data till some timeout happens in the system, blocking copy-paste > > feature and possibly freezing applications. > > > > A way to reproduce is copy-paste big clipboard data between two spice > > clients. > > > > Also add some documentation to the checks, in order to quickly > > understand what they are about. > > > > Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6 > > Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9 > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876 > > > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx> > > --- > > src/spice-gtk-session.c | 32 +++++++++++++++++++++++++++----- > > 1 file changed, 27 insertions(+), 5 deletions(-) > > > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c > > index 1a19bca..aa812d1 100644 > > --- a/src/spice-gtk-session.c > > +++ b/src/spice-gtk-session.c > > @@ -1015,12 +1015,30 @@ static gboolean clipboard_request(SpiceMainChannel *main, guint selection, > > int m; > > > > cb = get_clipboard_from_selection(s, selection); > > - g_return_val_if_fail(cb != NULL, FALSE); > > - g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE); > > - g_return_val_if_fail(s->clip_grabbed[selection], FALSE); > > + if (cb == NULL) { > > + goto notify_agent; > > + } > > > > - if (read_only(self)) > > - return FALSE; > > + /* As we are holding clipboard data sent by selection-grab from agent, the > > + * selection-request of clipboard data from client OS must fail. We either > > from client OS? Here it's a signal handler for guest selection-request. Yes, comment is wrong, should be: /* As we are holding clipboard data sent by selection-grab from guest, the * selection-request of clipboard data from guest must fail. We either * sent a bad selection-grab to the guest or the agent is buggy. */ > This would clearly be a guest-side bug if we reach that > condition (events are processed in order, and > clipboard_by_guest is set synchronously). Not true. Possible client-side bug, reproducible on X11, as I discussed before. As mentioned in the comment, we are sending wrong selection-grab to the guest. > Not sure sending a reply is going to help much in that case... It helps. Instead of blocking the application, it fails to paste the clipboard and all is good. > > + * sent a bad selection-grab to the agent or the agent is buggy. */ > > + if (s->clipboard_by_guest[selection]) { > > + SPICE_DEBUG("clipboard: agent request: grab on hold by agent, possible race"); > > + goto notify_agent; > > + } > > + > > + /* The selection-request by agent should happen only if the clipboard data is set > > + * by client */ > > + if (!s->clip_grabbed[selection]) { > > + SPICE_DEBUG("clipboard: agent request: data set by agent, possible race"); > > + goto notify_agent; > > + } > > This could be adding more race conditions. clip_grabbed is set > asynchronously after owner changed, and indicate if the grab > message was sent to the agent, We send a selection-release on owner-change. If we receive a selection-request before agent receives the selection-release, this is expected but we should notify the guest, afaics. > as you correctly say. It doesn't mean you can't request client > clipboard content. > I understand the racy case, but the condition seems wrong, it > should attempt to request current client clipboard content, and > fail/succeed after. No. It should request the content from the grab that was sent. If clipboard changed, previous grab gets invalidated and a selection-release is sent and another selection-grab is sent with the metadata of the new grab. Proving recent data from old grab seems wrong. > > > + /* Ready only, still should reply to agent to avoid it waiting for data */ > > No, read-only shouldn't reply. We are lacking a way to tell the guest > that the client is read-only though. So this may be acceptable for > now, but we should have a TODO/FIXME.. Well, it should not happen anyway as on read-only we should not send selection-grab. I'll fix and resend. Thanks for the quick review. > > + if (read_only(self)) { > > + g_warning("clipboard: agent request: read only, deny request"); > > + goto notify_agent; > > + } > > > > if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) { > > gtk_clipboard_request_text(cb, clipboard_received_text_cb, > > @@ -1039,6 +1057,10 @@ static gboolean clipboard_request(SpiceMainChannel *main, guint selection, > > } > > > > return TRUE; > > + > > +notify_agent: > > + spice_main_channel_clipboard_selection_notify(s->main, selection, type, NULL, 0); > > + return FALSE; > > } > > > > static void clipboard_release(SpiceMainChannel *main, guint selection, > > -- > > 2.20.1 > > > > _______________________________________________ > > Spice-devel mailing list > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > > > -- > Marc-André Lureau
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel