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

Submitted by Victor Toso on Jan. 10, 2019, 12:47 p.m.

Details

Message ID 20190110124714.7018-4-victortoso@redhat.com
State New
Headers show
Series "gtk-session and clipboard in x11" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Jan. 10, 2019, 12:47 p.m.
From: Victor Toso <me@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:

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.

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@redhat.com>
---
 src/spice-gtk-session.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Patch hide | download patch | download mbox

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. */
+    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
+        spice_gtk_session_get_keyboard_has_focus(self)) {
+        SPICE_DEBUG("clipboard get: not requesting data while holding focus");
+        return;
+    }
+#endif
+    SPICE_DEBUG("clipboard get: has-foucus=%d",
+                spice_gtk_session_get_keyboard_has_focus(self));
+
     selection = get_selection_from_clipboard(s, clipboard);
     g_return_if_fail(selection != -1);
     g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
@@ -818,6 +831,8 @@  static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
                                guint32* types, guint32 ntypes,
                                gpointer user_data)
 {
+    SPICE_DEBUG("clipboard_grab");
+
     g_return_val_if_fail(SPICE_IS_GTK_SESSION(user_data), FALSE);
 
     SpiceGtkSession *self = user_data;

Comments

Hi

On Thu, Jan 10, 2019 at 4:47 PM Victor Toso <victortoso@redhat.com> wrote:
>
> From: Victor Toso <me@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?

>
> 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@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.

> +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> +        spice_gtk_session_get_keyboard_has_focus(self)) {
> +        SPICE_DEBUG("clipboard get: not requesting data while holding focus");
> +        return;
> +    }
> +#endif
> +    SPICE_DEBUG("clipboard get: has-foucus=%d",
> +                spice_gtk_session_get_keyboard_has_focus(self));
> +
>      selection = get_selection_from_clipboard(s, clipboard);
>      g_return_if_fail(selection != -1);
>      g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
> @@ -818,6 +831,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
>                                 guint32* types, guint32 ntypes,
>                                 gpointer user_data)
>  {
> +    SPICE_DEBUG("clipboard_grab");
> +
>      g_return_val_if_fail(SPICE_IS_GTK_SESSION(user_data), FALSE);
>
>      SpiceGtkSession *self = user_data;
> --
> 2.20.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
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@redhat.com> wrote:
> >
> > From: Victor Toso <me@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

> > 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@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

Thanks for reviewing,
Victor

> 
> > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > +        SPICE_DEBUG("clipboard get: not requesting data while holding focus");
> > +        return;
> > +    }
> > +#endif
> > +    SPICE_DEBUG("clipboard get: has-foucus=%d",
> > +                spice_gtk_session_get_keyboard_has_focus(self));
> > +
> >      selection = get_selection_from_clipboard(s, clipboard);
> >      g_return_if_fail(selection != -1);
> >      g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
> > @@ -818,6 +831,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> >                                 guint32* types, guint32 ntypes,
> >                                 gpointer user_data)
> >  {
> > +    SPICE_DEBUG("clipboard_grab");
> > +
> >      g_return_val_if_fail(SPICE_IS_GTK_SESSION(user_data), FALSE);
> >
> >      SpiceGtkSession *self = user_data;
> > --
> > 2.20.1
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> 
> -- 
> Marc-André Lureau
Hi

On Thu, Jan 10, 2019 at 5:50 PM Victor Toso <victortoso@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@redhat.com> wrote:
> > >
> > > From: Victor Toso <me@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 :)

>
> > > 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@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/.

>
> Thanks for reviewing,
> Victor
>
> >
> > > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > > +        SPICE_DEBUG("clipboard get: not requesting data while holding focus");
> > > +        return;
> > > +    }
> > > +#endif
> > > +    SPICE_DEBUG("clipboard get: has-foucus=%d",
> > > +                spice_gtk_session_get_keyboard_has_focus(self));
> > > +
> > >      selection = get_selection_from_clipboard(s, clipboard);
> > >      g_return_if_fail(selection != -1);
> > >      g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
> > > @@ -818,6 +831,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> > >                                 guint32* types, guint32 ntypes,
> > >                                 gpointer user_data)
> > >  {
> > > +    SPICE_DEBUG("clipboard_grab");
> > > +
> > >      g_return_val_if_fail(SPICE_IS_GTK_SESSION(user_data), FALSE);
> > >
> > >      SpiceGtkSession *self = user_data;
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> >
> >
> > --
> > Marc-André Lureau
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@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@redhat.com> wrote:
> > > >
> > > > From: Victor Toso <me@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@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?

If the problem is having this that we should have this not only
on x11 but on wayland too, let me know.
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@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@redhat.com> wrote:
> > > > >
> > > > > From: Victor Toso <me@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@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?

Cheers,
Hi

On Thu, Jan 10, 2019 at 8:15 PM Victor Toso <victortoso@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@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@redhat.com> wrote:
> > > > > >
> > > > > > From: Victor Toso <me@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@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?