[Spice-devel] [spice-gtk v1 2/4] gtk-session: prefer early check to agent connectivity

[Victor Toso]

Hi,

On Thu, Dec 06, 2018 at 04:15:14AM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me at victortoso.com>
> > 
> > In case the agent is disconnected, we we don't need to create the
> > struct RunInfo, GMainLoop and add handlers to some signals.
> > 
> > This patch also removes one goto and related cleanup label.
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/spice-gtk-session.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 8d31045..1615172 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -725,6 +725,12 @@ static void clipboard_get(GtkClipboard *clipboard,
> >      g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
> >      g_return_if_fail(s->main != NULL);
> >  
> > +    g_object_get(s->main, "agent-connected", &agent_connected, NULL);
> > +    if (!agent_connected) {
> > +        SPICE_DEBUG("Request to guest failed as agent is not running");
> > +        return;
> > +    }
> > +
> >      ri.selection_data = selection_data;
> >      ri.info = info;
> >      ri.loop = g_main_loop_new(NULL, FALSE);
> > @@ -741,13 +747,6 @@ static void clipboard_get(GtkClipboard *clipboard,
> >      spice_main_channel_clipboard_selection_request(s->main, selection,
> >                                                     atom2agent[info].vdagent);
> >  
> > -
> > -    g_object_get(s->main, "agent-connected", &agent_connected, NULL);
> > -    if (!agent_connected) {
> > -        SPICE_DEBUG("canceled clipboard_get, before running loop");
> > -        goto cleanup;
> > -    }
> > -
> >      /* This is modeled on the implementation of gtk_dialog_run() even though
> >       * these thread functions are deprecated and appears to be needed to
> >       avoid
> >       * dead-lock from gtk_dialog_run().
> > @@ -758,7 +757,6 @@ static void clipboard_get(GtkClipboard *clipboard,
> >      gdk_threads_enter();
> >      G_GNUC_END_IGNORE_DEPRECATIONS
> >  
> > -cleanup:
> >      g_clear_pointer(&ri.loop, g_main_loop_unref);
> >      g_signal_handler_disconnect(s->main, clipboard_handler);
> >      g_signal_handler_disconnect(s->main, agent_handler);
> 
> Before spice_main_channel_clipboard_selection_request was
> always called so spice_channel_wakeup was also called. Honestly
> I don't have enough knowledge to understand if this could be an
> issue or not.

Ah, you are right. Well:
fn spice_main_channel_clipboard_selection_request() calls
fn agent_clipboard_request() which will fail on check
g_return_if_fail(c->agent_connected)

So, if we don't want to have the failure (we want to send the
agent message only if agent is connected) moving this check up is
not just a cleanup but should avoid this kind of issue.

At this moment, we don't have this check so _should_ be possible
to triggered this critical by calling
spice_gtk_session_paste_from_guest while agent is not
connected. Spicy tool listens to agent-connect and does not allow
it to happen but we shouldn't rely on the library's user, I
think.

> Not clear which events could be possible pending here, the
> function looks quite an hack, looking
> at the signals set there's "notify::agent-connected" which
> seems to mean that meanwhile the "agent-connected" value is
> expected to change during this function.
> 
> Frediano

Yeah, perhaps utility function would be better
(..)_agent_is_connected();

Victor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20181206/c3f60541/attachment.sig>