[Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus

[Jakub Janku]

Hi,

On Thu, Mar 14, 2019 at 6:18 PM Marc-André Lureau
<marcandre.lureau at gmail.com> wrote:
>
> Hi
>
> On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků <jjanku at redhat.com> wrote:
> >
> > If two grab messages in opposite directions "meet" on their way
> > to their destinations, we end up in a state when both spice-gtk
> > and spice-vdagent think that the other component can provide
> > clipboard data. As a consequence of this, neither of them can.
> >
> > This is a bug in the spice-protocol. To mitigate the issue,
> > accept grab only from the side that currently has keyboard focus,
> > this means:
> > (1) spice-gtk has focus ==>
> >     * accept grabs from vdagent,
> >     * ignore grabs from other apps running in the host
> > (2) spice-gtk doesn't have focus ==>
> >     * accept grabs from other apps running in the host
> >     * ignore grabs from vdagent
> >
> > The check in clipboard_owner_change() is X11-specific,
> > because on Wayland, spice-gtk is first notified about the
> > owner-change once it gains focus.
> >
> > The check in clipboard_grab() can be generic.
> > Note that on Wayland, calling gtk_clipboard_set_with_owner()
> > while not having focus doesn't have any effect anyway,
> > only focused clients can set clipboard.
> >
> > With this patch, the race can still occur around the time
> > when focus changes (rare, but possible), on X11 as well as Wayland.
> >
> > Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> >
> > Signed-off-by: Jakub Janků <jjanku at redhat.com>
> > ---
> >
> > Victor, half of this patch is basically yours,
> > so feel free to add signed-off or something,
> > in the case this gets upstream :)
> >
> > ---
> >  src/spice-gtk-session.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index b48f92a..7b7370c 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> >          s->clip_hasdata[selection] = FALSE;
> >          return;
> >      }
> > +
> > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > +        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> > +                    "ignoring grab from other app", __FUNCTION__);
> > +        return;
> > +    }
> >  #endif
>
> This will break clipboard managers interactions, which may take the
> clipboard content, save it and/or modify it.

Depends on the implementation of the given clipboard manager, I'd say.
I tried the "clipboard indicator" you're using. Seems like no problem there :)
>
> >
> >      s->clip_hasdata[selection] = TRUE;
> > @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> >      cb = get_clipboard_from_selection(s, selection);
> >      g_return_val_if_fail(cb != NULL, FALSE);
> >
> > +    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> > +        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> > +                    "ignoring grab from guest agent", __FUNCTION__);
> > +        return FALSE;
> > +    }
>
>
> Beside automation, the cursor alone may easily create a new clipboard
> content which won't be available to the client side (the auto-grab may
> fail to follow cursor etc).
>
> It's a bit unclear why it's not X11 specific but for client side
> change it is, this could deserve a code comment.

Tried to describe that in the commit log. I could add a comment in the
code as well.
>
> All in all, this feels weak and breaks some legitimate cases.
>
> I am not very strongly against this, as I understand it may help with
> some races we discussed,

Is this an ack or nack?
Seems like we're just going round in circles now...

> but it feels like the problem is elsewhere

sorry, but that's very vague, I have no idea what you're referring to

> and we need a better solution to prevent the race from happening.
>
> I haven't read the bug reports: this kind of workaround needs a
> description of a broken use case (not a theoretical description of a
> race that "never" happen in practice).

I described a broken case in the previous email:
KDE with klipper, "prevent empty clipboard" enabled + slow network

Cheers,
Jakub

>
>
> > +
> >      for (n = 0; n < ntypes; ++n) {
> >          found = FALSE;
> >          for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> > --
> > 2.20.1
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau