[Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure

[Marc-André Lureau]

Hi

On Wed, Jan 16, 2019 at 11:44 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
>
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/spice-gtk-session.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 1a19bca..aa812d1 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -1015,12 +1015,30 @@ 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;
> +    }
>
> -    if (read_only(self))
> -        return FALSE;
> +    /* As we are holding clipboard data sent by selection-grab from agent, the
> +     * selection-request of clipboard data from client OS must fail. We either

from client OS? Here it's a signal handler for guest selection-request.

This would clearly be a guest-side bug if we reach that condition
(events are processed in order, and clipboard_by_guest is set
synchronously). Not sure sending a reply is going to help much in that
case...

> +     * sent a bad selection-grab to the agent 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;
> +    }
> +
> +    /* 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 could be adding more race conditions. clip_grabbed is set
asynchronously after owner changed, and indicate if the grab message
was sent to the agent, as you correctly say. It doesn't mean you can't
request client clipboard content.

I understand the racy case, but the condition seems wrong, it should
attempt to request current client clipboard content, and fail/succeed
after.

> +    /* Ready only, still should reply to agent to avoid it waiting for data */

No, read-only shouldn't reply. We are lacking a way to tell the guest
that the client is read-only though. So this may be acceptable for
now, but we should have a TODO/FIXME..

> +    if (read_only(self)) {
> +        g_warning("clipboard: agent request: read only, deny request");
> +        goto notify_agent;
> +    }
>
>      if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
>          gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> @@ -1039,6 +1057,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



-- 
Marc-André Lureau