[Spice-devel] [spice-gtk v4 2/2] gtk-session: clipboard: x11: owner-change improvement

Tue Jan 15 09:25:22 UTC 2019

Hi,

On Tue, Jan 15, 2019 at 01:40:48AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jan 15, 2019 at 12:40 AM Jakub Janku <jjanku at redhat.com> wrote:
> >
> > Hi,
> >
> > On Mon, Jan 14, 2019 at 1:41 PM Marc-André Lureau
> > <marcandre.lureau at gmail.com> wrote:
> > >
> > > Hi
> > >
> > > On Mon, Jan 14, 2019 at 4:34 PM Victor Toso <victortoso at redhat.com> wrote:
> > > >
> > > > From: Victor Toso <me at victortoso.com>
> > > >
> > > > On X11, the release-grab message might end up clearing the
> > > > GtkClipboard which triggers the owner-changed callback having the
> > > > event owner as NULL.
> > > >
> > > > We should not be calling gtk_clipboard_request_targets() in this
> > > > situation as is prone to errors as the intention here is request
> > > > clipboard information from changes made by client OS.
> > > >
> > > > The fix is to avoid any such request while spice client is holding the
> > > > keyboard grab.
> > >
> > > Two things that bug me about this:
> > > - it's x11 specific, weird
> >
> > What's so weird about that?
> > It's also X11-specific that we can receive these "owner-change" events
> > while spice-gtk is holding the focus since on Wayland, apps need to
> > gain focus first to be able to set the clipboard, afaik.
> > The clipboard protocols on X11 and Wayland have simply been designed
> > in different ways. So although GTK tries to unify the behavior and
> > provide the same experience in both environments, some limitations
> > just cannot be overcome.
> 
> The patch is not about integration with X11, the way for
> example we have overlay, key locks, or mouse acceleration code
> that gtk doesn't provide. It's about an API that gtk+ provides
> that should be uniform.  Sometime it fails to provide the
> require abstraction. In this case, I don't undestand *yet* why
> the code should be specific to X11, it could be equally "valid"
> on wayland or windows etc...

My first attempt to make it generic was a failure because, on
Wayland, when we receive the owner-change event, we already had
the focus and would miss the event due my buggy code. I have it
fixed now in a more generic way, I have yet to test on windows.

> > > - the condition seems wrong: if an application has the keyboard grab,
> > > that doesn't mean that another cannot update the clipboard. For
> > > example, I suppose this can easily happen with multiple
> > > pointers/inputs or some automation.
> >
> > Sure, another app can update the clipboard, but imagine the following situation:
> > 1) user copies something in the guest
> > 2) an app in the client's system grabs the clipboard without having
> > keyboard focus -- this means that the grab likely wasn't directly
> > initiated by the user
> 
> I don't know, you could have some automation framework changing the
> clipboard. For testing, for example, or to remove swear words, or
> other kind of protected content..

Would be okay if we propose a gtk-session property to
enable/disable clipboard managers? If that's a use case you see
important, having it as a command line option should be enough

    eg: remote-viewer --allow-clipboard-manager

That would keep the code behavior as is.

> And I think you could have multiple keyboard/pointers on the same
> seat, although this may be esoteric, I agree.
> 
> > 3) spice-gtk receives "owner-change" event and subsequently sends grab
> > to the agent
> > 4) based on the grab message from spice-gtk in step 3), vdagent grabs
> > the clipboard in the guest system -- and by that, it overwrites the
> > clipboard contents copied in step 1)
> 
> That would be what I expect indeed.
> 
> >
> > So the clipboard is suddenly changed, although the user didn't take any action.
> 
> And this may also be what the user wanted.

I think the implication of 'not being what user wanted' be quite
severe. e.g: The user having his password stored in clipboard
temporally would have that sent to all guests that it is
connected at that moment.

> > I think this is unwanted and this patch solves it, so I'm all for
> > ack-ing this just for this reason. If this also solves the bug, it's
> > even better.
> >
> > Nevertheless, I would be really happy if we could track down what
> > exactly causes the issues that have been reported and describe it in
> > more detail.
> 
> Yes, that sounds like a better course to me.

Sorry, really not sure how to write it better myself. I tried.

I'm not taking in consideration that this is misbehavior of gtk
but expected behavior that we are not handling well, that's all.

> > > Other than that, if it fixes or workaround real clipboard issues, I am
> > > not against it, but I think we should add more comments about the
> > > "hack".
> > >
> > >
> > > >
> > > > 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
> > > >
> > > > Changed in v4:
> > > > - Updated commit log
> > > >
> > > > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > > > Tested-by: James Harvey @jamespharvey20
> >
> > Does this mean it fixes the issues for him?

Yes, some replies and also private chat with feedback.

    https://lists.freedesktop.org/archives/spice-devel/2018-December/046556.html

> > > > ---
> > > >  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 abce43f..20df70d 100644
> > > > --- a/src/spice-gtk-session.c
> > > > +++ b/src/spice-gtk-session.c
> > > > @@ -674,6 +674,19 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> > > >          return;
> > > >      }
> > > >
> > > > +#ifdef GDK_WINDOWING_X11
> > > > +    /* In X11, while holding the keyboard-grab we are not interested in this
> > > > +     * event as it either came by grab or release messages from agent.  */
> >
> > For completeness, it might be good to mention that the event can be
> > caused by other applications too (as Marc-André pointed out), even
> > though spice-gtk is holding the keyboard-grab.

Well, that can be extended but it is duplicated doc. Here I'm
trying to point out why we are about to ignore the owner-change
event, that is, "while holding the keyboard-grab...". I tried to
put the whole scenario above the function.

I'll change the code nevertheless, if next version is worse than
this one, I can resend this with better info.

> >
> > > > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > > > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > > > +        SPICE_DEBUG("clipboard: owner-changed event: not requesting client's target "
> > > > +                    "while holding focus");
> > > > +        return;
> > > > +    }
> > > > +#endif
> > > > +    SPICE_DEBUG("clipboard: owner-changed event: has-focus=%d",
> > > > +                spice_gtk_session_get_keyboard_has_focus(self));
> > > > +
> > > >      s->clipboard_by_guest[selection] = FALSE;
> > > >      s->clip_hasdata[selection] = TRUE;
> > > >      if (s->auto_clipboard_enable && !read_only(self))
> > > > --
> > > > 2.20.1
> > > >
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > >
> > Cheers,
> > Jakub

Many thanks,
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/20190115/8d9ae54e/attachment-0001.sig>