Hi, On Wed, Jan 16, 2019 at 10:11 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 > > v1 -> v2: > - Fixed comment on s->clipboard_by_guest[selection] check (Marc-André) > - Removed the reply on read only. (Marc-André) > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx> > --- > src/spice-gtk-session.c | 29 +++++++++++++++++++++++++---- > 1 file changed, 25 insertions(+), 4 deletions(-) > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c > index 1a19bca..82a5b35 100644 > --- a/src/spice-gtk-session.c > +++ b/src/spice-gtk-session.c > @@ -1015,12 +1015,29 @@ 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; > + } cb == NULL means that the requested selection can't have been advertised by spice-gtk, so the vdagent shouldn't request the selection's data. This seems like a clear bug in vdagent to me. Should we respond? I'm afraid it might potentially result into some bugs staying unnoticed in the vdagent. > + > + /* 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. */ > + if (s->clipboard_by_guest[selection]) { > + SPICE_DEBUG("clipboard: agent request: grab on hold by agent, possible race"); > + goto notify_agent; > + } Yes, this can happen, this is the bug we're trying to fix. But this change doesn't really fix it, it just mitigates it. As you're saying in the comment, it's a bug either in the client or in the agent. I'd rather fix the bug than try to alleviate its "symptoms". > > - if (read_only(self)) > + /* 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 can surely happen and it wouldn't be a programmer error, so the g_return_val_if_fail() should indeed be replaced by a less severe log. But again, I have some doubts whether we should respond. The new clipboard code in linux vdagent clears all pending requests upon release message. So sending the data message would actually produce a warning on the guest side as the corresponding request wouldn't be found. I'm not sure how the Windows agent handles this. > + > + /* On read only, should not reply to the agent. */ > + if (read_only(self)) { > return FALSE; > + } > > if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) { > gtk_clipboard_request_text(cb, clipboard_received_text_cb, > @@ -1039,6 +1056,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 Cheers, Jakub _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel