[Spice-devel] [spice-gtk v2] gtk-session: do not request guest's clipboard data unnecessarily
Jakub Janku
jjanku at redhat.com
Tue Jan 8 17:18:16 UTC 2019
On Tue, Jan 8, 2019 at 10:06 AM Victor Toso <victortoso at redhat.com> wrote:
>
> 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.
Makes sense.
>
> > 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)
Correct.
>
> > > +
> > > 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
This is only a debug message that is logged when the --spice-debug
option is used, so the verbosity shouldn't be a problem, or is it?
> 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.
Personally, I would move this change in logging to another patch as it
isn't really related to the fix itself, but I'll leave it up to you.
Acked-by: Jakub Janků <jjanku at redhat.com>
>
> > > 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
More information about the Spice-devel
mailing list