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

[Frediano Ziglio]

> 
> 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.

> +}
> +
> +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