[Spice-devel] [spice-gtk v2] gtk-session: do not request guest's clipboard data unnecessarily
Victor Toso
victortoso at redhat.com
Tue Jan 8 09:06:46 UTC 2019
Hi,
Thanks for review!
On Mon, Jan 07, 2019 at 09:28:04PM +0100, Jakub Janku wrote:
> Hi,
>
> On Mon, Jan 7, 2019 at 1:41 PM Victor Toso <victortoso at redhat.com> wrote:
> >
> > From: Victor Toso <me at victortoso.com>
> >
> > If SpiceGtkSession is holding the keyboard, that's huge indication
> > that the client should not be requesting guest's clipboard data yet.
> >
> > This patch adds a check in clipboard_get() callback, to avoid such
> > requests. In Linux, this only happens with X11 backend.
> >
> > This patch helps to handle a possible state race between who owns the
> > grab between client and agent which could lead to agent clipboard
> > failing or getting stuck, see:
> >
> > Linux guest:
> > https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> >
> > Windows guest:
> > https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> >
> > The way to reproduce the race might depend on guest system and
> > applications but it is connected to amount of VDAGENTD_CLIPBOARD_GRAB
> > sent by the agent which depends on the amount of clipboard changes in
> > the guest. Simple example is on RHEL 6.10, with Gedit, select a text
> > char by char; Client receives VDAGENTD_CLIPBOARD_GRAB every time a new
> > char is selected instead of once when the full message is selected.
> >
> > v1 -> v2:
> > * Moved the check to the right place, otherwise the patch would not
> > work on Wayland (Christophe, Jakub)
> > * Improve commit log (Jakub)
>
> Now I get it, thanks :)
> >
> > 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 at redhat.com>
> > ---
> > src/spice-gtk-session.c | 26 ++++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 1ccae07..a78f619 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -59,6 +59,7 @@ struct _SpiceGtkSessionPrivate {
> > gboolean clip_hasdata[CLIPBOARD_LAST];
> > gboolean clip_grabbed[CLIPBOARD_LAST];
> > gboolean clipboard_by_guest[CLIPBOARD_LAST];
> > + gboolean clipboard_by_guest_released[CLIPBOARD_LAST];
> > /* auto-usbredir related */
> > gboolean auto_usbredir_enable;
> > int auto_usbredir_reqs;
> > @@ -634,6 +635,12 @@ static void clipboard_owner_change(GtkClipboard *clipboard,
> > if (s->main == NULL)
> > return;
> >
> > + if (s->clipboard_by_guest_released[selection]) {
> > + /* Ignore owner-change event if this is just about agent releasing grab */
> > + s->clipboard_by_guest_released[selection] = FALSE;
> > + return;
> > + }
> > +
> > if (s->clip_grabbed[selection]) {
> > s->clip_grabbed[selection] = FALSE;
> > if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > @@ -728,6 +735,14 @@ static void clipboard_get(GtkClipboard *clipboard,
> > g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
> > g_return_if_fail(s->main != NULL);
> >
> > + /* No real need to set clipboard data while no client application will
> > + * be requested. This is useful for clients on X11 as in Wayland, this
> > + * callback is only called when SpiceGtkSession:keyboard-focus is false */
> > + if (spice_gtk_session_get_keyboard_has_focus(self)) {
> > + SPICE_DEBUG("Do not request clipboard data while holding the keyboard focus");
> > + return;
> > + }
>
> Have you tested this with some clipboard managers? I would
> expect them to request the clipboard data as soon as they
> receive a new owner-change event.
I haven't but considering how Wayland works and the rationale
behind it, I don't think we should care much if that might be a
problem to other applications while our application is holding
the keyboard grab. As the owner of the clipboard still is Spice
when user leaves to another application (keyboard grab is lost)
the request for clipboard data should succeed then.
> So this patch may potentially cripple them, but I might be
> wrong. Not sure whether this is a use-case we need to support
> but it might be good to know the consequences of this patch.
I don't think you are wrong that might affect clipboard managers
but only if they are poking to every change that happens in the
clipboard (like spice-vdagent on X11)
> > +
> > ri.selection_data = selection_data;
> > ri.info = info;
> > ri.loop = g_main_loop_new(NULL, FALSE);
> > @@ -769,6 +784,15 @@ cleanup:
> >
> > static void clipboard_clear(GtkClipboard *clipboard, gpointer user_data)
> > {
> > + SpiceGtkSession *self = user_data;
> > + SpiceGtkSessionPrivate *s = self->priv;
> > + gint selection = get_selection_from_clipboard(s, clipboard);
> > +
> > + g_return_if_fail(selection != -1);
> > +
> > + if (s->clipboard_by_guest_released[selection])
> > + return;
> > +
> This gave me a pause for a while. It seems rather weird to me
> that only part of the clipboard code logs debug messages (e.g.
> clipboard_get_targets() prints all the atoms but there's no
> logging in clipboard_grab()). By bypassing the SPICE_DEBUG
> below, we would lose track of the guest's clipboard release
> event as there's no log in clipboard_release() either.
>
> Maybe it would be useful to add some logging to the other
> functions as well? (clipboard_{grab, request, release})
Yes, I have some clipboard cleanup patches to do, did not want to
mix. I thought a bit about keeping clipboard_clear() log with
this events but it can be quite verbose while user is just
interacting with the guest; also, as that will be ignored by
owner-event, the clipboard_clear log below is likely a non
ignored event, hopefully more useful in the logs.
I can remove the check and leave the log for all, no problem with
that too. Some cleanup is needed with our without it.
> > SPICE_DEBUG("clipboard_clear");
> > /* We watch for clipboard ownership changes and act on those, so we
> > don't need to do anything here */
> > @@ -1035,6 +1059,8 @@ static void clipboard_release(SpiceMainChannel *main, guint selection,
> >
> > if (!s->clipboard_by_guest[selection])
> > return;
> > +
> > + s->clipboard_by_guest_released[selection] = TRUE;
> > gtk_clipboard_clear(clipboard);
> > s->clipboard_by_guest[selection] = FALSE;
> > }
> > --
> > 2.20.1
> >
>
> Otherwise seems good to me, but I haven't tested it.
>
> Cheers,
> Jakub
Thanks again,
Victor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190108/45b0f864/attachment.sig>
More information about the Spice-devel
mailing list