[Spice-devel] [PATCH spice-server 1/2] server/red_channel: fix possible access to released channel clients

Alon Levy alevy at redhat.com
Mon Jun 4 05:26:02 PDT 2012


On Mon, Jun 04, 2012 at 11:46:02AM +0200, Christophe Fergeau wrote:
> On Thu, May 31, 2012 at 10:04:05AM +0300, Yonit Halperin wrote:
> > Added ref count for RedChannel and RedChannelClient.
> 
> Just a side note, longer term we should probably introduce a
> SpiceRefCounted base class which would contain a refcount and a destructor
> function, and then we'd be able to use generic ref/unref functions on the
> various objects instead of always reimplementing it. One way of doing that
> would be to use gobjects, but I don't know how well that would fit within
> qemu.

That will fit just fine with qemu, that is using glib for 1.1 and I
think since 1.0, so no added dependency.

> 
> Christophe
> 
> > 
> > red_channel.c/red_peer_handle_incoming call to
> > handler->cb->handle_message might lead to the release of the channel
> > client, and the following call to handler->cb->release_msg_buf will be
> > a violation.
> > 
> > This bug can be produced by causing main_channel_handle_parsed
> > call red_client_destory, e.g., by some violation in
> > reds_on_main_agent_data that will result in a call to reds_disconnect.
> > ---
> >  server/red_channel.c |  119 +++++++++++++++++++++++++++++++++++++++++--------
> >  server/red_channel.h |    5 ++
> >  2 files changed, 104 insertions(+), 20 deletions(-)
> > 
> > diff --git a/server/red_channel.c b/server/red_channel.c
> > index 83a9f37..de50047 100644
> > --- a/server/red_channel.c
> > +++ b/server/red_channel.c
> > @@ -43,6 +43,45 @@ static void red_client_remove_channel(RedChannelClient *rcc);
> >  static RedChannelClient *red_client_get_channel(RedClient *client, int type, int id);
> >  static void red_channel_client_restore_main_sender(RedChannelClient *rcc);
> >  
> > +/*
> > + * Lifetime of RedChannel, RedChannelClient and RedClient:
> > + * RedChannel is created and destroyed by the calls to
> > + * red_channel_create.* and red_channel_destroy. The RedChannel resources
> > + * are deallocated only after red_channel_destroy is called and no RedChannelClient
> > + * refers to the channel.
> > + * RedChannelClient is created and destroyed by the calls to red_channel_client_create
> > + * and red_channel_client_destroy. RedChannelClient resources are deallocated only when
> > + * its refs == 0. The reference count of RedChannelClient can be increased by routines
> > + * that include calls that might destroy the red_channel_client. For example,
> > + * red_peer_handle_incoming calls the handle_message proc of the channel, which
> > + * might lead to destroying the client. However, after the call to handle_message,
> > + * there is a call to the channel's release_msg_buf proc.
> > + *
> > + * Once red_channel_client_destroy is called, the RedChannelClient is disconnected and
> > + * removed from the RedChannel clients list, but if rcc->refs != 0, it will still hold
> > + * a reference to the Channel. The reason for this is that on the one hand RedChannel holds
> > + * callbacks that may be still in use by RedChannel, and on the other hand,
> > + * when an operation is performed on the list of clients that belongs to the channel,
> > + * we don't want to execute it on the "to be destroyed" channel client.
> > + *
> > + * RedClient is created and destroyed by the calls to red_client_new and red_client_destroy.
> > + * When it is destroyed, it also disconnects and destroys all the RedChannelClients that
> > + * are associated with it. However, since part of these channel clients may still have
> > + * other references, they will not be completely released, until they are dereferenced.
> > + *
> > + * Note: red_channel_client_destroy is not thread safe, and still it is called from
> > + * red_client_destroy (from the client's thread). However, since before this call,
> > + * red_client_destroy calls rcc->channel->client_cbs.disconnect(rcc), which is synchronous,
> > + * we assume that if the channel is in another thread, it does no longer have references to
> > + * this channel client.
> > + * If a call to red_channel_client_destroy is made from another location, it must be called
> > + * from the channel's thread.
> > +*/
> > +static void red_channel_ref(RedChannel *channel);
> > +static void red_channel_unref(RedChannel *channel);
> > +static void red_channel_client_ref(RedChannelClient *rcc);
> > +static void red_channel_client_unref(RedChannelClient *rcc);
> > +
> >  static uint32_t full_header_get_msg_size(SpiceDataHeaderOpaque *header)
> >  {
> >      return ((SpiceDataHeader *)header->data)->size;
> > @@ -245,7 +284,9 @@ static void red_peer_handle_incoming(RedsStream *stream, IncomingHandler *handle
> >  
> >  void red_channel_client_receive(RedChannelClient *rcc)
> >  {
> > +    red_channel_client_ref(rcc);
> >      red_peer_handle_incoming(rcc->stream, &rcc->incoming);
> > +    red_channel_client_unref(rcc);
> >  }
> >  
> >  void red_channel_receive(RedChannel *channel)
> > @@ -541,6 +582,7 @@ RedChannelClient *red_channel_client_create(int size, RedChannel *channel, RedCl
> >      rcc->stream = stream;
> >      rcc->channel = channel;
> >      rcc->client = client;
> > +    rcc->refs = 1;
> >      rcc->ack_data.messages_window = ~0;  // blocks send message (maybe use send_data.blocked +
> >                                               // block flags)
> >      rcc->ack_data.client_generation = ~0;
> > @@ -585,6 +627,7 @@ RedChannelClient *red_channel_client_create(int size, RedChannel *channel, RedCl
> >      rcc->id = channel->clients_num;
> >      red_channel_add_client(channel, rcc);
> >      red_client_add_channel(client, rcc);
> > +    red_channel_ref(channel);
> >      pthread_mutex_unlock(&client->lock);
> >      return rcc;
> >  error:
> > @@ -628,6 +671,7 @@ RedChannel *red_channel_create(int size,
> >      channel = spice_malloc0(size);
> >      channel->type = type;
> >      channel->id = id;
> > +    channel->refs = 1;
> >      channel->handle_acks = handle_acks;
> >      memcpy(&channel->channel_cbs, channel_cbs, sizeof(ChannelCbs));
> >  
> > @@ -693,6 +737,7 @@ RedChannel *red_channel_create_dummy(int size, uint32_t type, uint32_t id)
> >      channel = spice_malloc0(size);
> >      channel->type = type;
> >      channel->id = id;
> > +    channel->refs = 1;
> >      channel->core = &dummy_core;
> >      ring_init(&channel->clients);
> >      client_cbs.connect = red_channel_client_default_connect;
> > @@ -790,6 +835,50 @@ void red_channel_set_data(RedChannel *channel, void *data)
> >      channel->data = data;
> >  }
> >  
> > +static void red_channel_ref(RedChannel *channel)
> > +{
> > +    channel->refs++;
> > +}
> > +
> > +static void red_channel_unref(RedChannel *channel)
> > +{
> > +    if (!--channel->refs) {
> > +        if (channel->local_caps.num_common_caps) {
> > +            free(channel->local_caps.common_caps);
> > +        }
> > +
> > +        if (channel->local_caps.num_caps) {
> > +            free(channel->local_caps.caps);
> > +        }
> > +
> > +        free(channel);
> > +    }
> > +}
> > +
> > +static void red_channel_client_ref(RedChannelClient *rcc)
> > +{
> > +    rcc->refs++;
> > +}
> > +
> > +static void red_channel_client_unref(RedChannelClient *rcc)
> > +{
> > +    if (!--rcc->refs) {
> > +        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)
> >  {
> >      rcc->destroying = 1;
> > @@ -797,16 +886,7 @@ void red_channel_client_destroy(RedChannelClient *rcc)
> >          red_channel_client_disconnect(rcc);
> >      }
> >      red_client_remove_channel(rcc);
> > -    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);
> > -    free(rcc);
> > +    red_channel_client_unref(rcc);
> >  }
> >  
> >  void red_channel_destroy(RedChannel *channel)
> > @@ -822,15 +902,7 @@ void red_channel_destroy(RedChannel *channel)
> >              SPICE_CONTAINEROF(link, RedChannelClient, channel_link));
> >      }
> >  
> > -    if (channel->local_caps.num_common_caps) {
> > -        free(channel->local_caps.common_caps);
> > -    }
> > -
> > -    if (channel->local_caps.num_caps) {
> > -        free(channel->local_caps.caps);
> > -    }
> > -
> > -    free(channel);
> > +    red_channel_unref(channel);
> >  }
> >  
> >  void red_channel_client_shutdown(RedChannelClient *rcc)
> > @@ -845,7 +917,9 @@ void red_channel_client_shutdown(RedChannelClient *rcc)
> >  
> >  void red_channel_client_send(RedChannelClient *rcc)
> >  {
> > +    red_channel_client_ref(rcc);
> >      red_peer_handle_outgoing(rcc->stream, &rcc->outgoing);
> > +    red_channel_client_unref(rcc);
> >  }
> >  
> >  void red_channel_send(RedChannel *channel)
> > @@ -886,7 +960,7 @@ void red_channel_client_push(RedChannelClient *rcc)
> >      } else {
> >          return;
> >      }
> > -
> > +    red_channel_client_ref(rcc);
> >      if (rcc->send_data.blocked) {
> >          red_channel_client_send(rcc);
> >      }
> > @@ -900,6 +974,7 @@ void red_channel_client_push(RedChannelClient *rcc)
> >          red_channel_client_send_item(rcc, pipe_item);
> >      }
> >      rcc->during_send = FALSE;
> > +    red_channel_client_unref(rcc);
> >  }
> >  
> >  void red_channel_push(RedChannel *channel)
> > @@ -997,12 +1072,14 @@ static void red_channel_client_event(int fd, int event, void *data)
> >  {
> >      RedChannelClient *rcc = (RedChannelClient *)data;
> >  
> > +    red_channel_client_ref(rcc);
> >      if (event & SPICE_WATCH_EVENT_READ) {
> >          red_channel_client_receive(rcc);
> >      }
> >      if (event & SPICE_WATCH_EVENT_WRITE) {
> >          red_channel_client_push(rcc);
> >      }
> > +    red_channel_client_unref(rcc);
> >  }
> >  
> >  void red_channel_client_init_send_data(RedChannelClient *rcc, uint16_t msg_type, PipeItem *item)
> > @@ -1246,8 +1323,10 @@ RedChannelClient *red_channel_client_create_dummy(int size,
> >          goto error;
> >      }
> >      rcc = spice_malloc0(size);
> > +    rcc->refs = 1;
> >      rcc->client = client;
> >      rcc->channel = channel;
> > +    red_channel_ref(channel);
> >      red_channel_client_set_remote_caps(rcc, num_common_caps, common_caps, num_caps, caps);
> >      if (red_channel_client_test_remote_common_cap(rcc, SPICE_COMMON_CAP_MINI_HEADER)) {
> >          rcc->incoming.header = mini_header_wrapper;
> > diff --git a/server/red_channel.h b/server/red_channel.h
> > index 765b74e..e77e484 100644
> > --- a/server/red_channel.h
> > +++ b/server/red_channel.h
> > @@ -225,6 +225,9 @@ struct RedChannelClient {
> >      RedChannel *channel;
> >      RedClient  *client;
> >      RedsStream *stream;
> > +
> > +    uint32_t refs;
> > +
> >      struct {
> >          uint32_t generation;
> >          uint32_t client_generation;
> > @@ -268,6 +271,8 @@ struct RedChannel {
> >      uint32_t type;
> >      uint32_t id;
> >  
> > +    uint32_t refs;
> > +
> >      RingItem link; // channels link for reds
> >  
> >      SpiceCoreInterface *core;
> > -- 
> > 1.7.7.6
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel



> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel



More information about the Spice-devel mailing list