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. 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 sure sending a reply is going to help much in that case... > + * 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, 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. > + /* 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.. > + 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel