[Spice-devel] [spice-gtk v2] gtk-session: clipboard request: notify agent on failure

[Jakub Janku]

Hi,

On Wed, Jan 16, 2019 at 10:11 AM Victor Toso <victortoso at redhat.com> wrote:
>
> From: Victor Toso <me at victortoso.com>
>
> Similar to 172c521271a3d - if we don't, the agent might be waiting for
> data till some timeout happens in the system, blocking copy-paste
> feature and possibly freezing applications.
>
> A way to reproduce is copy-paste big clipboard data between two spice
> clients.
>
> Also add some documentation to the checks, in order to quickly
> understand what they are about.
>
> 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
>
> v1 -> v2:
> - Fixed comment on s->clipboard_by_guest[selection] check (Marc-André)
> - Removed the reply on read only. (Marc-André)
>
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/spice-gtk-session.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 1a19bca..82a5b35 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -1015,12 +1015,29 @@ static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
>      int m;
>
>      cb = get_clipboard_from_selection(s, selection);
> -    g_return_val_if_fail(cb != NULL, FALSE);
> -    g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE);
> -    g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
> +    if (cb == NULL) {
> +        goto notify_agent;
> +    }

cb == NULL means that the requested selection can't have been
advertised by spice-gtk, so the vdagent shouldn't request the
selection's data.
This seems like a clear bug in vdagent to me. Should we respond? I'm
afraid it might potentially result into some bugs staying unnoticed in
the vdagent.

> +
> +    /* As we are holding clipboard data sent by selection-grab from guest, the
> +     * selection-request of clipboard data from guest must fail. We either sent
> +     * a bad selection-grab to the guest or the agent is buggy. */
> +    if (s->clipboard_by_guest[selection]) {
> +        SPICE_DEBUG("clipboard: agent request: grab on hold by agent, possible race");
> +        goto notify_agent;
> +    }

Yes, this can happen, this is the bug we're trying to fix. But this
change doesn't really fix it, it just mitigates it.
As you're saying in the comment, it's a bug either in the client or in
the agent. I'd rather fix the bug than try to alleviate its
"symptoms".

>
> -    if (read_only(self))
> +    /* The selection-request by agent should happen only if the clipboard data is set
> +     * by client */
> +    if (!s->clip_grabbed[selection]) {
> +        SPICE_DEBUG("clipboard: agent request: data set by agent, possible race");
> +        goto notify_agent;
> +    }

This can surely happen and it wouldn't be a programmer error, so the
g_return_val_if_fail() should indeed be replaced by a less severe log.

But again, I have some doubts whether we should respond. The new
clipboard code in linux vdagent clears all pending requests upon
release message. So sending the data message would actually produce a
warning on the guest side as the corresponding request wouldn't be
found. I'm not sure how the Windows agent handles this.

> +
> +    /* On read only, should not reply to the agent. */
> +    if (read_only(self)) {
>          return FALSE;
> +    }
>
>      if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
>          gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> @@ -1039,6 +1056,10 @@ static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
>      }
>
>      return TRUE;
> +
> +notify_agent:
> +    spice_main_channel_clipboard_selection_notify(s->main, selection, type, NULL, 0);
> +    return FALSE;
>  }
>
>  static void clipboard_release(SpiceMainChannel *main, guint selection,
> --
> 2.20.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Cheers,
Jakub