> > 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). Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel