[Spice-devel] [spice-gtk v3 3/3] gtk-session: clipboard: x11: do not request data while on focus

[Marc-André Lureau]

Hi

On Thu, Jan 10, 2019 at 8:15 PM Victor Toso <victortoso at redhat.com> wrote:
>
> Hi,
>
> On Thu, Jan 10, 2019 at 04:26:13PM +0100, Victor Toso wrote:
> > Hi,
> >
> > On Thu, Jan 10, 2019 at 07:01:59PM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Thu, Jan 10, 2019 at 5:50 PM Victor Toso <victortoso at redhat.com> wrote:
> > > >
> > > > On Thu, Jan 10, 2019 at 05:25:21PM +0400, Marc-André Lureau wrote:
> > > > > Hi
> > > > >
> > > > > On Thu, Jan 10, 2019 at 4:47 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:
> > > > >
> > > > > hmm
> > > > >
> > > > > > 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.
> > > > >
> > > > > Sorry, you get a lot of clipboard-grab from the remote, but where is
> > > > > the problem?
> > > >
> > > > Problem is, why fetch data that no other application is
> > > > requesting.
> > > >
> > > > Situations like the one below happens due state changing fast +
> > > > idle callbacks for clear/request clipboard data...
> > > >
> > > >     https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6#note_85246
> > >
> > > Sorry, I don't understand the problem, yes I try :)
> >
> > I don't follow. You try to reproduce and failed?
> >
> > I could erase the mentioning of the bugs and etc and just leave
> > it as..
> >
> >   | Problem is, why fetch data that no other application is
> >   | requesting.
> >
> > .. and would be enough for me.
> >
> > > > > > v2 -> v3:
> > > > > > * Split the fix in two patches while adding some info in a 3rd patch;
> > > > > > * Kept the "clipboard_clear" as it was (Jakub)
> > > > > > * Added a "clipboard_grab" log in clipboard_grab() function;
> > > > > >
> > > > > > 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 | 15 +++++++++++++++
> > > > > >  1 file changed, 15 insertions(+)
> > > > > >
> > > > > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > > > > index 85d5880..f5959f7 100644
> > > > > > --- a/src/spice-gtk-session.c
> > > > > > +++ b/src/spice-gtk-session.c
> > > > > > @@ -763,6 +763,19 @@ static void clipboard_get(GtkClipboard *clipboard,
> > > > > >
> > > > > >      SPICE_DEBUG("clipboard get");
> > > > > >
> > > > > > +#ifdef GDK_WINDOWING_X11
> > > > > > +    /* Do not request clipboard data while we are still interacting with the
> > > > > > +     * Guest; Clipboard data could change shortly and this request would just
> > > > > > +     * be wasteful. */
> > > > >
> > > > > I don't think there is anything preventing another app the
> > > > > clipboard data while spice-gtk has the focus. In fact, that's
> > > > > how clipboard manager work afaik.
> > > >
> > > > That means that another application in Client OS might get remote
> > > > clipboard data while the user is interacting with remote VM.
> > > > That's pretty much a security concern as well. This does not
> > > > happen on Wayland and I would rather that we avoid it with X11
> > > > too, even if it breaks ClipboardManagers that don't request our
> > > > clipboard after we focus-out
> > >
> > > I think it could happen on wayland as well, for example with
> > > https://extensions.gnome.org/extension/779/clipboard-indicator/.
> >
> > I could not install it to test it, sadly. Any other app in mind?
>
> GPaste did the job, indeed same goes for wayland. I really
> thought it would not allow other application to grab clipboard
> data.
>
> > If the problem is having this that we should have this not only
> > on x11 but on wayland too, let me know.
>
> I removed the check for x11 and what I consider a success
> happens, GPaste does not get clipboard data while we are
> interacting in the VM.
>
> If you don't like that approach, another solution is to postpone
> the set_with_owner till we focus-out?

I would like to understand correctly the race first, before discussing
a solution. The gedit case you mentioned works fine here, is this
happening very rarely? Could you describe the sequence of events?

-- 
Marc-André Lureau