On Fri, Sep 07, 2018 at 04:40:16AM -0400, Frediano Ziglio wrote: > > > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > The channel_connect() function could fail leading to a spice-channel > > existing as zombie (its coroutine return soon after). > > > > Check if channel_connect() fails and give a proper error signal to > > user when that happens. > > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1625550 > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx> > > --- > > src/spice-channel.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/src/spice-channel.c b/src/spice-channel.c > > index 0a5437c..48e3265 100644 > > --- a/src/spice-channel.c > > +++ b/src/spice-channel.c > > @@ -2675,8 +2675,12 @@ cleanup: > > if (c->state == SPICE_CHANNEL_STATE_RECONNECTING || > > c->state == SPICE_CHANNEL_STATE_SWITCHING) { > > g_warn_if_fail(c->event == SPICE_CHANNEL_NONE); > > - channel_connect(channel, c->tls); > > - g_object_unref(channel); > > + if (channel_connect(channel, c->tls)) { > > + g_object_unref(channel); > > + } else { > > + c->event = SPICE_CHANNEL_ERROR_CONNECT; > > + g_idle_add(spice_channel_delayed_unref, data); > > + } > > } else > > g_idle_add(spice_channel_delayed_unref, data); > > > > Reading at all "cleanup" code is a bit confusing. > The comment "Co-routine exits now - the SpiceChannel object "... > actually happens only if you call g_object_unref, so only > after calling channel_connect but the comment is in more paths. > I would instead add a "return NULL" line after g_object_unref > so to make code clear even without the comment, remove the comment > and the else too, something like: > > > diff --git a/src/spice-channel.c b/src/spice-channel.c > index d1a72fc..2506431 100644 > --- a/src/spice-channel.c > +++ b/src/spice-channel.c > @@ -2677,11 +2677,10 @@ cleanup: > g_warn_if_fail(c->event == SPICE_CHANNEL_NONE); > channel_connect(channel, c->tls); > g_object_unref(channel); > - } else > - g_idle_add(spice_channel_delayed_unref, data); > + return NULL; > + } > > - /* Co-routine exits now - the SpiceChannel object may no longer exist, > - so don't do anything else now unless you like SEGVs */ > + g_idle_add(spice_channel_delayed_unref, channel); > return NULL; > } > > > This would make this patch something like > > > diff --git a/src/spice-channel.c b/src/spice-channel.c > index 2506431..e56ec35 100644 > --- a/src/spice-channel.c > +++ b/src/spice-channel.c > @@ -2675,9 +2675,11 @@ cleanup: > if (c->state == SPICE_CHANNEL_STATE_RECONNECTING || > c->state == SPICE_CHANNEL_STATE_SWITCHING) { > g_warn_if_fail(c->event == SPICE_CHANNEL_NONE); > - channel_connect(channel, c->tls); > - g_object_unref(channel); > - return NULL; > + if (channel_connect(channel, c->tls)) { > + g_object_unref(channel); > + return NULL; > + } > + c->event = SPICE_CHANNEL_ERROR_CONNECT; > } > > g_idle_add(spice_channel_delayed_unref, channel); > > > Note also that channel_connect returns true also if client > provided the socket. In this case g_object_unref will be > called from spice_channel_coroutine (so coroutine context) > which apparently from spice_channel_delayed_unref comment you > want to avoid (unless you are sure there's another reference > that prevents real object destruction). That would require host configuration to use socket. Neither of SPICE_CHANNEL_STATE_RECONNECTING and SPICE_CHANNEL_STATE_SWITCHING should happen in this scenario but I could add a sanity check on spice_session_get_client_provided_socket(c->session); I'll take your suggestion but keeping the SEGVs comment ;) I'll send v2 later today. Thanks for the review, Victor
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel