[Spice-devel] [PATCH 2/5] channel: do not free rcc->stream in red_channel_client_disconnect
Christophe Fergeau
cfergeau at redhat.com
Fri Oct 23 07:44:42 PDT 2015
On Fri, Oct 16, 2015 at 07:23:07AM -0400, Frediano Ziglio wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> >
> > This fixes the following scary racy corruption after glib loop switch:
> >
> > ==28173==
> > ==28173== Debugger has detached. Valgrind regains control. We
> > continue.
> > ==28173== Invalid read of size 8
> > ==28173== at 0x4C7871E: reds_stream_read (reds.c:4521)
> > ==28173== by 0x4C2F9D7: red_peer_receive (red_channel.c:209)
> > ==28173== by 0x4C2FB59: red_peer_handle_incoming (red_channel.c:255)
> > ==28173== by 0x4C2FF36:
> > red_channel_client_receive (red_channel.c:329)
> > ==28173== by 0x4C33D6D: red_channel_client_event (red_channel.c:1577)
> > ==28173== by 0x4C65098: watch_func (red_worker.c:10292)
> > ==28173== by 0x504DDAB: g_io_unix_dispatch (giounix.c:167)
> > ==28173== by 0x4FFACB6: g_main_dispatch (gmain.c:3065)
> > ==28173== by 0x4FFBA0D: g_main_context_dispatch (gmain.c:3641)
> > ==28173== by 0x4FFBBFF: g_main_context_iterate (gmain.c:3712)
> > ==28173== by 0x4FFC028: g_main_loop_run (gmain.c:3906)
> > ==28173== by 0x4C6ABF2: red_worker_main (red_worker.c:12180)
> > ==28173== Address 0x7d688e0 is 32 bytes inside a block of size 160
> > free'd
> > ==28173== at 0x4A074C4: free (in
> > /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> > ==28173== by 0x4C78A84: reds_stream_free (reds.c:4594)
> > ==28173== by 0x4C34E2E:
> > red_channel_client_disconnect (red_channel.c:1865)
> > ==28173== by 0x4C30363:
> > red_channel_client_default_peer_on_error (red_channel.c:417)
> > ==28173== by 0x4C3011B: red_peer_handle_outgoing (red_channel.c:372)
> > ==28173== by 0x4C3305F: red_channel_client_send (red_channel.c:1298)
> > ==28173== by 0x4C33FB6:
> > red_channel_client_begin_send_message (red_channel.c:1616)
> > ==28173== by 0x4C5F4B4:
> > display_begin_send_message (red_worker.c:8360)
> > ==28173== by 0x4C61DE8: display_channel_send_item (red_worker.c:9164)
> > ==28173== by 0x4C30C69:
> > red_channel_client_send_item (red_channel.c:599)
> > ==28173== by 0x4C332BB: red_channel_client_push (red_channel.c:1351)
> > ==28173== by 0x4C33C3A:
> > red_channel_client_handle_message (red_channel.c:1545)
> > ---
> > server/red_channel.c | 39 +++++++++++++++++++++------------------
> > server/red_worker.c | 2 ++
> > 2 files changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/server/red_channel.c b/server/red_channel.c
> > index 8db3d6e..4d5066a 100644
> > --- a/server/red_channel.c
> > +++ b/server/red_channel.c
> > @@ -1233,22 +1233,25 @@ static void red_channel_client_ref(RedChannelClient
> > *rcc)
> >
> > static void red_channel_client_unref(RedChannelClient *rcc)
> > {
> > - if (!--rcc->refs) {
> > - spice_debug("destroy rcc=%p", rcc);
> > - if (rcc->send_data.main.marshaller) {
> > - spice_marshaller_destroy(rcc->send_data.main.marshaller);
> > - }
> > + spice_debug("rcc=%p", rcc);
> >
> > - if (rcc->send_data.urgent.marshaller) {
> > - spice_marshaller_destroy(rcc->send_data.urgent.marshaller);
> > - }
> > + if (--rcc->refs)
> > + return;
> >
> > - red_channel_client_destroy_remote_caps(rcc);
> > - if (rcc->channel) {
> > - red_channel_unref(rcc->channel);
> > - }
> > - free(rcc);
> > - }
> > + reds_stream_free(rcc->stream);
> > + rcc->stream = NULL;
> > +
> > + if (rcc->send_data.main.marshaller)
> > + spice_marshaller_destroy(rcc->send_data.main.marshaller);
> > +
> > + if (rcc->send_data.urgent.marshaller)
> > + spice_marshaller_destroy(rcc->send_data.urgent.marshaller);
> > +
> > + red_channel_client_destroy_remote_caps(rcc);
> > + if (rcc->channel)
> > + red_channel_unref(rcc->channel);
> > +
> > + free(rcc);
> > }
> >
>
> This part looks quite a big change but mainly is style one.
> However the debug string is printed at every reference change and not only
> when really released.
> I would rollback all these style changes.
Or they can be split in a separate patch. They are definitely not very
good here as they hide non-cosmetic changes (the addition of
reds_stream_free())
>
> > void red_channel_client_destroy(RedChannelClient *rcc)
> > @@ -1758,7 +1761,7 @@ void red_channel_pipes_add_empty_msg(RedChannel
> > *channel, int msg_type)
> > int red_channel_client_is_connected(RedChannelClient *rcc)
> > {
> > if (!rcc->dummy) {
> > - return rcc->stream != NULL;
> > + return (rcc->stream != NULL) &&
> > ring_item_is_linked(&rcc->channel_link);
> > } else {
> > return rcc->dummy_connected;
> > }
> > @@ -1813,8 +1816,10 @@ static void red_channel_remove_client(RedChannelClient
> > *rcc)
> > rcc->channel->type, rcc->channel->id,
> > rcc->channel->thread_id, pthread_self());
> > }
> > + spice_return_if_fail(ring_item_is_linked(&rcc->channel_link));
> > +
> > ring_remove(&rcc->channel_link);
> > - spice_assert(rcc->channel->clients_num > 0);
> > + spice_return_if_fail(rcc->channel->clients_num > 0);
>
> I think this WAS detecting an invalid internal state and the check was basically
> made silent. Any reason?
Why 'made silent' ? There is still a runtime log message when this is
triggered, I assume the caller will still be working fine even if this
method returned early?
>
> > rcc->channel->clients_num--;
> > // TODO: should we set rcc->channel to NULL???
> > }
> > @@ -1854,8 +1859,6 @@ void red_channel_client_disconnect(RedChannelClient
> > *rcc)
> > rcc->channel->core->watch_remove(rcc->stream->watch);
> > rcc->stream->watch = NULL;
> > }
> > - reds_stream_free(rcc->stream);
> > - rcc->stream = NULL;
> > if (rcc->latency_monitor.timer) {
> > rcc->channel->core->timer_remove(rcc->latency_monitor.timer);
> > rcc->latency_monitor.timer = NULL;
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 6cc5a35..742edfc 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -10135,6 +10135,8 @@ static gboolean watch_func(GIOChannel *source,
> > GIOCondition condition,
> > SpiceWatch *watch = data;
> > int fd = g_io_channel_unix_get_fd(source);
> >
> > + spice_return_val_if_fail(!g_source_is_destroyed(watch->source), FALSE);
> > +
>
> Should glib call this function if the object is destroyed?
Might be some sanity check added during debugging.
Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151023/0788bdb9/attachment.sig>
More information about the Spice-devel
mailing list