[Spice-devel] [PATCH spice-server v2] Manage lifetime of RedVmcChannel

[Victor Toso]

Hi,

On Wed, Nov 02, 2016 at 04:34:47AM -0400, Frediano Ziglio wrote:
> > 
> > From: Jonathon Jongsma <jjongsma at redhat.com>
> > 
> > Store the channel in the char device object, and unref it in dispose.
> > Previously, we were just creating a channel and allowing it to leak.
> > There may be better long-term solutions, but this works for now.
> > ---
> >  server/spicevmc.c | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > Changes:
> > - squashed fixup.
> > 
> > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > index 7067bc7..a77e868 100644
> > --- a/server/spicevmc.c
> > +++ b/server/spicevmc.c
> > @@ -47,6 +47,9 @@
> >  #define BUF_SIZE (64 * 1024 + 32)
> >  #define COMPRESS_THRESHOLD 1000
> >  
> > +typedef struct RedVmcChannel RedVmcChannel;
> > +typedef struct RedVmcChannelClass RedVmcChannelClass;
> > +
> >  typedef struct RedVmcPipeItem {
> >      RedPipeItem base;
> >  
> > @@ -75,6 +78,7 @@ typedef struct RedCharDeviceSpiceVmcClass
> > RedCharDeviceSpiceVmcClass;
> >  
> >  struct RedCharDeviceSpiceVmc {
> >      RedCharDevice parent;
> > +    RedVmcChannel *channel;
> >  };
> >  
> >  struct RedCharDeviceSpiceVmcClass
> > @@ -89,9 +93,6 @@ static RedCharDevice
> > *red_char_device_spicevmc_new(SpiceCharDeviceInstance *sin,
> >  
> >  G_DEFINE_TYPE(RedCharDeviceSpiceVmc, red_char_device_spicevmc,
> >  RED_TYPE_CHAR_DEVICE)
> >  
> > -#define RED_CHAR_DEVICE_SPICEVMC_PRIVATE(o) \
> > -    (G_TYPE_INSTANCE_GET_PRIVATE ((o), RED_TYPE_CHAR_DEVICE_SPICEVMC,
> > RedCharDeviceSpiceVmcPrivate))
> > -
> >  #define RED_TYPE_VMC_CHANNEL red_vmc_channel_get_type()
> >  
> >  #define RED_VMC_CHANNEL(obj) \
> > @@ -103,9 +104,6 @@ G_DEFINE_TYPE(RedCharDeviceSpiceVmc,
> > red_char_device_spicevmc, RED_TYPE_CHAR_DEV
> >  #define RED_VMC_CHANNEL_GET_CLASS(obj) \
> >      (G_TYPE_INSTANCE_GET_CLASS((obj), RED_TYPE_VMC_CHANNEL,
> >      RedVmcChannelClass))
> >  
> > -typedef struct RedVmcChannel RedVmcChannel;
> > -typedef struct RedVmcChannelClass RedVmcChannelClass;
> > -
> >  struct RedVmcChannel
> >  {
> >      RedChannel parent;
> > @@ -855,9 +853,13 @@ RedCharDevice *spicevmc_device_connect(RedsState *reds,
> >                                         SpiceCharDeviceInstance *sin,
> >                                         uint8_t channel_type)
> >  {
> > +    RedCharDeviceSpiceVmc *dev_state;
> >      RedVmcChannel *state = red_vmc_channel_new(reds, channel_type, sin);
> >  
> > -    return state->chardev;
> > +    dev_state = RED_CHAR_DEVICE_SPICEVMC(state->chardev);
> > +    dev_state->channel = state;
> > +
> > +    return RED_CHAR_DEVICE(dev_state);
> >  }
> >  
> >  /* Must be called from RedClient handling thread. */
> > @@ -904,10 +906,21 @@ SPICE_GNUC_VISIBLE void
> > spice_server_port_event(SpiceCharDeviceInstance *sin, ui
> >  }
> >  
> >  static void
> > +red_char_device_spicevmc_dispose(GObject *object)
> > +{
> > +    RedCharDeviceSpiceVmc *self = RED_CHAR_DEVICE_SPICEVMC(object);
> > +
> > +    g_clear_object(&self->channel);
>
> This call produce a double free.
> The channel is already freed in spicevmc_device_disconnect.

But g_clear_object() verify if channel is null before the unref and set
it to NULL afterwards.

>
> > +}
> > +
> > +static void
> >  red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcClass *klass)
> >  {
> > +    GObjectClass *object_class = G_OBJECT_CLASS(klass);
> >      RedCharDeviceClass *char_dev_class = RED_CHAR_DEVICE_CLASS(klass);
> >
> > +    object_class->dispose = red_char_device_spicevmc_dispose;
> > +
> >      char_dev_class->read_one_msg_from_device =
> >      spicevmc_chardev_read_msg_from_dev;
> >      char_dev_class->send_msg_to_client =
> >      spicevmc_chardev_send_msg_to_client;
> >      char_dev_class->send_tokens_to_client =
> >      spicevmc_char_dev_send_tokens_to_client;
>
> Removing the double free from this patch will basically revert the
> entire patch except the RED_CHAR_DEVICE_SPICEVMC_PRIVATE cleanup.
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161102/8e02dac5/attachment.sig>