Re: [PATCH 05/10] Add SmartCardChannelClientPrivate struct

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> Hi,
> 
> On Wed, Aug 31, 2016 at 11:54:41AM -0500, Jonathon Jongsma wrote:
> > Prepare to port to GObject by encapsulating all private data
> 
> If I'm not mistaken, the private struct now will leak while
> SmartCardChannelClient is not a GObject with its finalize/dispose.
> If that is the case, might point it out in the commit log and/or a FIXME
> on g_new0
> 

This was why I used the priv[1] "trick", no allocation required, no free,
allocation and "initialization" of priv is done while base object is
allocated like in GObject.
Considering that possibly that patches are going in a 0.14 version I
would surely avoid the leak.

Frediano

> Besides that, looks ok to me.
> Reviewed-by: Victor Toso <victortoso@xxxxxxxxxx>
> 
> > ---
> >  server/smartcard.c | 87
> >  +++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 47 insertions(+), 40 deletions(-)
> >
> > diff --git a/server/smartcard.c b/server/smartcard.c
> > index 74c2b18..8d22be4 100644
> > --- a/server/smartcard.c
> > +++ b/server/smartcard.c
> > @@ -49,8 +49,14 @@
> >  // Maximal length of APDU
> >  #define APDUBufSize 270
> >
> > +typedef struct SmartCardChannelClientPrivate
> > SmartCardChannelClientPrivate;
> >  typedef struct SmartCardChannelClient {
> >      RedChannelClient base;
> > +
> > +    SmartCardChannelClientPrivate *priv;
> > +} SmartCardChannelClient;
> > +
> > +struct SmartCardChannelClientPrivate {
> >      RedCharDeviceSmartcard *smartcard;
> >  
> >      /* read_from_client/write_to_device buffer.
> > @@ -58,7 +64,7 @@ typedef struct SmartCardChannelClient {
> >      RedCharDeviceWriteBuffer *write_buf;
> >      int msg_in_write_buf; /* was the client msg received into a
> >      RedCharDeviceWriteBuffer
> >                             * or was it explicitly malloced */
> > -} SmartCardChannelClient;
> > +};
> >  
> >  G_DEFINE_TYPE(RedCharDeviceSmartcard, red_char_device_smartcard,
> >  RED_TYPE_CHAR_DEVICE)
> >  
> > @@ -316,9 +322,9 @@ static void
> > smartcard_char_device_attach_client(SpiceCharDeviceInstance *char_de
> >      RedCharDeviceSmartcard *dev =
> >      red_char_device_opaque_get(char_device->st);
> >      int client_added;
> >  
> > -    spice_assert(!scc->smartcard && !dev->priv->scc);
> > +    spice_assert(!scc->priv->smartcard && !dev->priv->scc);
> >      dev->priv->scc = scc;
> > -    scc->smartcard = dev;
> > +    scc->priv->smartcard = dev;
> >      client_added = red_char_device_client_add(RED_CHAR_DEVICE(dev),
> >                                                red_channel_client_get_client(&scc->base),
> >                                                FALSE, /* no flow control
> >                                                yet */
> > @@ -330,7 +336,7 @@ static void
> > smartcard_char_device_attach_client(SpiceCharDeviceInstance *char_de
> >      if (!client_added) {
> >          spice_warning("failed");
> >          dev->priv->scc = NULL;
> > -        scc->smartcard = NULL;
> > +        scc->priv->smartcard = NULL;
> >          red_channel_client_disconnect(&scc->base);
> >      }
> >  }
> > @@ -361,14 +367,14 @@ static void
> > smartcard_char_device_detach_client(SmartCardChannelClient *scc)
> >  {
> >      RedCharDeviceSmartcard *dev;
> >  
> > -    if (!scc->smartcard) {
> > +    if (!scc->priv->smartcard) {
> >          return;
> >      }
> > -    dev = scc->smartcard;
> > +    dev = scc->priv->smartcard;
> >      spice_assert(dev->priv->scc == scc);
> >      red_char_device_client_remove(RED_CHAR_DEVICE(dev),
> >                                    red_channel_client_get_client(&scc->base));
> > -    scc->smartcard = NULL;
> > +    scc->priv->smartcard = NULL;
> >      dev->priv->scc = NULL;
> >  }
> >  
> > @@ -386,26 +392,26 @@ static uint8_t
> > *smartcard_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
> >      /* todo: only one reader is actually supported. When we fix the code
> >      to support
> >       * multiple readers, we will porbably associate different devices to
> >       * differenc channels */
> > -    if (!scc->smartcard) {
> > -        scc->msg_in_write_buf = FALSE;
> > +    if (!scc->priv->smartcard) {
> > +        scc->priv->msg_in_write_buf = FALSE;
> >          return spice_malloc(size);
> >      } else {
> >          RedCharDeviceSmartcard *dev;
> >  
> >          spice_assert(g_smartcard_readers.num == 1);
> > -        dev = scc->smartcard;
> > -        spice_assert(dev->priv->scc || scc->smartcard);
> > -        spice_assert(!scc->write_buf);
> > -        scc->write_buf =
> > red_char_device_write_buffer_get(RED_CHAR_DEVICE(dev),
> > -
> > red_channel_client_get_client(rcc),
> > -                                                          size);
> > -
> > -        if (!scc->write_buf) {
> > +        dev = scc->priv->smartcard;
> > +        spice_assert(dev->priv->scc || scc->priv->smartcard);
> > +        spice_assert(!scc->priv->write_buf);
> > +        scc->priv->write_buf =
> > red_char_device_write_buffer_get(RED_CHAR_DEVICE(dev),
> > +
> > red_channel_client_get_client(rcc),
> > +                                                                size);
> > +
> > +        if (!scc->priv->write_buf) {
> >              spice_error("failed to allocate write buffer");
> >              return NULL;
> >          }
> > -        scc->msg_in_write_buf = TRUE;
> > -        return scc->write_buf->buf;
> > +        scc->priv->msg_in_write_buf = TRUE;
> > +        return scc->priv->write_buf->buf;
> >      }
> >  }
> >  
> > @@ -420,13 +426,13 @@ static void
> > smartcard_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> >       * multiple readers, we will porbably associate different devices to
> >       * differenc channels */
> >  
> > -    if (!scc->msg_in_write_buf) {
> > -        spice_assert(!scc->write_buf);
> > +    if (!scc->priv->msg_in_write_buf) {
> > +        spice_assert(!scc->priv->write_buf);
> >          free(msg);
> >      } else {
> > -        if (scc->write_buf) { /* msg hasn't been pushed to the guest */
> > -            spice_assert(scc->write_buf->buf == msg);
> > -
> > red_char_device_write_buffer_release(RED_CHAR_DEVICE(scc->smartcard),
> > &scc->write_buf);
> > +        if (scc->priv->write_buf) { /* msg hasn't been pushed to the guest
> > */
> > +            spice_assert(scc->priv->write_buf->buf == msg);
> > +
> > red_char_device_write_buffer_release(RED_CHAR_DEVICE(scc->priv->smartcard),
> > &scc->priv->write_buf);
> >          }
> >      }
> >  }
> > @@ -467,7 +473,7 @@ static void
> > smartcard_channel_send_migrate_data(RedChannelClient *rcc,
> >      SpiceMarshaller *m2;
> >  
> >      scc = SPICE_CONTAINEROF(rcc, SmartCardChannelClient, base);
> > -    dev = scc->smartcard;
> > +    dev = scc->priv->smartcard;
> >      red_channel_client_init_send_data(rcc, SPICE_MSG_MIGRATE_DATA, item);
> >      spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_SMARTCARD_MAGIC);
> >      spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_SMARTCARD_VERSION);
> > @@ -513,8 +519,8 @@ static void
> > smartcard_channel_on_disconnect(RedChannelClient *rcc)
> >  {
> >      SmartCardChannelClient *scc = SPICE_CONTAINEROF(rcc,
> >      SmartCardChannelClient, base);
> >  
> > -    if (scc->smartcard) {
> > -        RedCharDeviceSmartcard *dev = scc->smartcard;
> > +    if (scc->priv->smartcard) {
> > +        RedCharDeviceSmartcard *dev = scc->priv->smartcard;
> >  
> >          smartcard_char_device_detach_client(scc);
> >          smartcard_char_device_notify_reader_remove(dev);
> > @@ -578,13 +584,13 @@ static void
> > smartcard_remove_reader(SmartCardChannelClient *scc, uint32_t reader
> >              VSC_GENERAL_ERROR);
> >          return;
> >      }
> > -    spice_assert(scc->smartcard == dev);
> > +    spice_assert(scc->priv->smartcard == dev);
> >      smartcard_char_device_notify_reader_remove(dev);
> >  }
> >  
> >  static void smartcard_add_reader(SmartCardChannelClient *scc, uint8_t
> >  *name)
> >  {
> > -    if (!scc->smartcard) { /* we already tried to attach a reader to the
> > client
> > +    if (!scc->priv->smartcard) { /* we already tried to attach a reader to
> > the client
> >                                      when it connected */
> >          SpiceCharDeviceInstance *char_device =
> >          smartcard_readers_get_unattached();
> >  
> > @@ -595,7 +601,7 @@ static void smartcard_add_reader(SmartCardChannelClient
> > *scc, uint8_t *name)
> >          }
> >          smartcard_char_device_attach_client(char_device, scc);
> >      }
> > -    smartcard_char_device_notify_reader_add(scc->smartcard);
> > +    smartcard_char_device_notify_reader_add(scc->priv->smartcard);
> >      // The device sends a VSC_Error message, we will let it through, no
> >      // need to send our own. We already set the correct reader_id, from
> >      // our RedCharDeviceSmartcard.
> > @@ -614,7 +620,7 @@ static void
> > smartcard_channel_write_to_reader(RedCharDeviceWriteBuffer *write_bu
> >      spice_assert(vheader->reader_id <= g_smartcard_readers.num);
> >      sin = g_smartcard_readers.sin[vheader->reader_id];
> >      dev = (RedCharDeviceSmartcard *)red_char_device_opaque_get(sin->st);
> > -    spice_assert(!dev->priv->scc || dev == dev->priv->scc->smartcard);
> > +    spice_assert(!dev->priv->scc || dev ==
> > dev->priv->scc->priv->smartcard);
> >      /* protocol requires messages to be in network endianess */
> >      vheader->type = htonl(vheader->type);
> >      vheader->length = htonl(vheader->length);
> > @@ -623,8 +629,8 @@ static void
> > smartcard_channel_write_to_reader(RedCharDeviceWriteBuffer *write_bu
> >      /* pushing the buffer to the write queue; It will be released
> >       * when it will be fully consumed by the device */
> >      red_char_device_write_buffer_add(sin->st, write_buf);
> > -    if (dev->priv->scc && write_buf == dev->priv->scc->write_buf) {
> > -        dev->priv->scc->write_buf = NULL;
> > +    if (dev->priv->scc && write_buf == dev->priv->scc->priv->write_buf) {
> > +        dev->priv->scc->priv->write_buf = NULL;
> >      }
> >  }
> >  
> > @@ -676,7 +682,7 @@ static int
> > smartcard_channel_client_handle_migrate_data(RedChannelClient *rcc,
> >          return TRUE;
> >      }
> >  
> > -    if (!scc->smartcard) {
> > +    if (!scc->priv->smartcard) {
> >          SpiceCharDeviceInstance *char_device =
> >          smartcard_readers_get_unattached();
> >  
> >          if (!char_device) {
> > @@ -687,10 +693,10 @@ static int
> > smartcard_channel_client_handle_migrate_data(RedChannelClient *rcc,
> >          }
> >      }
> >      spice_debug("reader added %d partial read_size %u",
> >      mig_data->reader_added, mig_data->read_size);
> > -    scc->smartcard->priv->reader_added = mig_data->reader_added;
> > +    scc->priv->smartcard->priv->reader_added = mig_data->reader_added;
> >  
> > -    smartcard_device_restore_partial_read(scc->smartcard, mig_data);
> > -    return red_char_device_restore(RED_CHAR_DEVICE(scc->smartcard),
> > &mig_data->base);
> > +    smartcard_device_restore_partial_read(scc->priv->smartcard, mig_data);
> > +    return red_char_device_restore(RED_CHAR_DEVICE(scc->priv->smartcard),
> > &mig_data->base);
> >  }
> >  
> >  static int smartcard_channel_handle_message(RedChannelClient *rcc,
> > @@ -737,8 +743,8 @@ static int
> > smartcard_channel_handle_message(RedChannelClient *rcc,
> >              vheader->type, vheader->length);
> >          return FALSE;
> >      }
> > -    spice_assert(scc->write_buf->buf == msg);
> > -    smartcard_channel_write_to_reader(scc->write_buf);
> > +    spice_assert(scc->priv->write_buf->buf == msg);
> > +    smartcard_channel_write_to_reader(scc->priv->write_buf);
> >  
> >      return TRUE;
> >  }
> > @@ -760,6 +766,7 @@ static void smartcard_connect_client(RedChannel
> > *channel, RedClient *client,
> >                                                                FALSE,
> >                                                                num_common_caps,
> >                                                                common_caps,
> >                                                                num_caps,
> >                                                                caps);
> > +    scc->priv = g_new0(SmartCardChannelClientPrivate,1 );
> >  
> >      if (!scc) {
> >          return;
> > @@ -818,7 +825,7 @@ red_char_device_smartcard_finalize(GObject *object)
> >  
> >      free(self->priv->buf);
> >      if (self->priv->scc) {
> > -        self->priv->scc->smartcard = NULL;
> > +        self->priv->scc->priv->smartcard = NULL;
> >      }
> >  
> >      G_OBJECT_CLASS(red_char_device_smartcard_parent_class)->finalize(object);
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]