Hi

On Wed, Jan 16, 2019 at 12:56 PM Victor Toso <victort...@redhat.com> wrote:
>
> Hi,
>
> On Wed, Jan 16, 2019 at 12:40:36PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Jan 16, 2019 at 11:44 AM Victor Toso <victort...@redhat.com> wrote:
> > >
> > > From: Victor Toso <m...@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 <victort...@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.
>
> Yes, comment is wrong, should be:
>
>     /* 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. */
>
> > 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 true. Possible client-side bug, reproducible on X11, as I
> discussed before. As mentioned in the comment, we are sending
> wrong selection-grab to the guest.

Sorry, I don't follow "we are sending wrong selection-grab" ?

>
> > Not sure sending a reply is going to help much in that case...
>
> It helps. Instead of blocking the application, it fails to paste
> the clipboard and all is good.

But if the agent logic is already wrong, sending a reply isn't going
to help, it could do anything..

>
> > > +     * 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,
>
> We send a selection-release on owner-change. If we receive a
> selection-request before agent receives the selection-release,
> this is expected but we should notify the guest, afaics.
>
> > 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.
>
> No. It should request the content from the grab that was sent. If

There is no such thing as clipboard ID/nth in any clipboard protocol I
know of. User simply request current clipboard content (not the nth),
whether it changed since last grab is not important: it will retrieve
something of the requested type or nothing.

> clipboard changed, previous grab gets invalidated and a
> selection-release is sent and another selection-grab is sent with
> the metadata of the new grab.
>
> Proving recent data from old grab seems wrong.

It is not.

> >
> > > +    /* 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..
>
> Well, it should not happen anyway as on read-only we should not
> send selection-grab. I'll fix and resend.

Right

>
> Thanks for the quick review.
>
> > > +    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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> >
> >
> > --
> > Marc-André Lureau



-- 
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Reply via email to