Re: [PATCH spice-server v2] Manage lifetime of RedVmcChannel

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

 



On Wed, 2016-11-02 at 13:05 -0400, Frediano Ziglio wrote:
> > 
> > 
> > On Wed, 2016-11-02 at 08:31 -0400, Frediano Ziglio wrote:
> > > 
> > > > 
> > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > On Wed, Nov 02, 2016 at 04:34:47AM -0400, Frediano Ziglio
> > > > wrote:
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > From: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > > > > > 
> > > > > > 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,
> > > > > >                                         SpiceCharDeviceInst
> > > > > > ance
> > > > > > *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.
> > > > 
> > > 
> > > Doing a NULL check on a dangling pointer is not much helpful.
> > 
> > Hmm, you're right. If we're going to remove the 'opaque' property
> > (in a
> > different patch), I think we do need to keep a pointer to the
> > channel
> > inside of the char device. But perhaps it should be a weak
> > reference
> > instead. So maybe we could just remove the dispose changes from
> > this
> > patch and squash it with that patch?
> > 
> 
> Yes, I think we should kind of merge part of this patch in the other
> one.
> This could also be fixed setting the pointer to NULL after calling
> spicevmc_device_disconnect.
> 
> > 
> > Also: perhaps the RedsState object should hold a reference to the
> > channels added via reds_register_channel() and unref the channel in
> > reds_unregister_channel(). It seems that it shouldn't be necessary
> > to
> > call red_channel_destroy() explicitly. This should just happen in
> > the
> > RedChannel destructor when the last ref is dropped...
> > 
> > 
> 
> There is already a patch in the queue that does this; you wrote it :)
> (cfr "RedsState: keep weak reference to channels").

Yeah, I actually wrote a follow-up email saying the exact same thing
right after I wrote the one you're quoting  ;) Except I forgot to send
it. It's been too long since I wrote these patches, I don't even
remember what they are anymore :)


> This patch does not seem really in line with your proposal for
> spice_server_destroy/reds_exit changes.

Yeah, perhaps I need to take a step back and evaluate things anew now
that we're basically at the end of the refactory merging. 

> 
> I was going to reply to it. There are some issues:
> - reds_channel_destroyed is called by RedChannel destructor
> (dispose/finalize);
>   As channels is not protected by a lock this means that all calls to
>   reds_channel_destroyed (so in theory all calls to g_object_unref to
>   RedChannel hierarchy) has to be done in the "main" thread... or we
> need to
>   add locking! It's easy to check reds_unregister_channel calls, a
> bit less
>   g_object_refs;
> - is it the right time to remove the item from the list or is too
> late?
>   Potentially the list is scanned when a new connection arrive to
> decide which
>   RedChannel will handle the connection (and create a new
> RedChannelClient).
>   Not saying that is wrong, surely will avoid dandling pointers but
> some race
>   may occur.
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > +}
> > > > > > +
> > > > > > +static void
> > > > > >  red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcC
> > > > > > lass
> > > > > > *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@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]