[Spice-devel] [PATCH] channel: do not free rcc->stream in red_channel_client_disconnect

[Frediano Ziglio]

> 
> Hi,
> 
> On Tue, Jan 19, 2016 at 09:52:24AM +0000, Frediano Ziglio wrote:
> > This fixes a crash if red_channel_client disconnect is called
> > handling a message.
> > This can happen for instance handling SPICE_MSGC_ACK which calls
> > red_channel_client_push which try to write items to socket detecting
> > writing errors (for instance socket disconnection).
> > Messages are read in a loop and the red_channel_client_disconnect
> > would cause rcc->stream to be NULL which will turn in a use-after-free
> > problem (stream in red_peer_handle_incoming will use cached stream value).
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-channel.c | 38 ++++++++++++++++++++++----------------
> >  1 file changed, 22 insertions(+), 16 deletions(-)
> > 
> > diff --git a/server/red-channel.c b/server/red-channel.c
> > index 306c87d..51e8958 100644
> > --- a/server/red-channel.c
> > +++ b/server/red-channel.c
> > @@ -1231,22 +1231,28 @@ void red_channel_client_ref(RedChannelClient *rcc)
> >  
> >  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);
> > -        }
> > +    if (--rcc->refs != 0) {
> 
> My opnion is that doing --(var) on if() does not make the code easy
> readable. rcc->refs will always be decreased so it should be outside
> the conditional.
> 
> I see this in multiple places in spice-server but my vote is to change
> that at least for new code :)
> 
> best,
>   toso
> 

I don't know I think is readable too. For me was readable also 
   if (--rcc->refs)
but time ago many agreed the " != 0" was improving readability

On the same hunk this is not part of the fix but just style. But
there has been multiple changes (less indentation) for *_unref
functions.

I'll post a version with smaller diff.

Frediano

> > +        return;
> > +    }
> >  
> > -        if (rcc->send_data.urgent.marshaller) {
> > -            spice_marshaller_destroy(rcc->send_data.urgent.marshaller);
> > -        }
> > +    spice_debug("destroy rcc=%p", rcc);
> >  
> > -        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);
> >  }
> >  
> >  void red_channel_client_destroy(RedChannelClient *rcc)
> > @@ -1749,7 +1755,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 ring_item_is_linked(&rcc->channel_link);
> >      } else {
> >          return rcc->dummy_connected;
> >      }
> > @@ -1804,6 +1810,8 @@ 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);
> >      rcc->channel->clients_num--;
> > @@ -1845,8 +1853,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;
> > --
> > 2.4.3
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>