[libvirt] [libvirt-glib] a leak in libvirt-glib

[Daniel P. Berrange]

On Mon, Feb 27, 2012 at 05:25:58PM +0100, Marc-André Lureau wrote:
> Hi,
> 
> In libvirt-glib, we call virStreamEventAddCallback() and give it a
> ref, that is supposed to be unref when the stream event is removed. But
> this doesn't happen! I tracked it down to:
> 
> static void remoteStreamCallbackFree(void *opaque)
> {
>     struct remoteStreamCallbackData *cbdata = opaque;
> 
>     if (!cbdata->cb && cbdata->ff)
>         (cbdata->ff)(cbdata->opaque);
> 
> Why are we checking for cbdata->cb here? That gives us a leak
> when taking screenshots. So far I solved it
> with the attached patch for libvirt-glib. I noticed it because that
> of a resulting process & fd leakage in the libvirtd side.
> 
> It might be that the fix should be in libvirt, but anyway, the proposed
> patch doesn't need a libvirt depedency update and also keeps the
> object reference manangement at the libvirt-glib level, which is nice.
> 
> 
> -- 
> Marc-André Lureau

> From e3ff31c92edd2390def3226647681cc7252c1a1a Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau at redhat.com>
> Date: Thu, 2 Feb 2012 21:22:42 +0100
> Subject: [PATCH libvirt-glib] Don't leak GVirStream
> 
> Do not give away a reference to virStreamEventAddCallback, but manage
> stream life-time and event instead.
> 
> It seems to be a bug in current implementation of libvirt
> remoteStreamCallbackFree() that doesn't call the free/notify cb for
> some reason. (even if it did, I am not sure it would work correctly,
> so I prefer that patch)
> ---
>  libvirt-gobject/libvirt-gobject-stream.c |   29 ++++++++++++++++++-----------
>  1 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/libvirt-gobject/libvirt-gobject-stream.c b/libvirt-gobject/libvirt-gobject-stream.c
> index a1039b1..5486b95 100644
> --- a/libvirt-gobject/libvirt-gobject-stream.c
> +++ b/libvirt-gobject/libvirt-gobject-stream.c
> @@ -67,6 +67,7 @@ enum {
>  
>  #define GVIR_STREAM_ERROR gvir_stream_error_quark()
>  
> +static void gvir_stream_update_events(GVirStream *stream);
>  
>  static GQuark
>  gvir_stream_error_quark(void)
> @@ -204,13 +205,6 @@ static void gvir_stream_finalize(GObject *object)
>      if (self->priv->input_stream)
>          g_object_unref(self->priv->input_stream);
>  
> -    if (priv->handle) {
> -        if (virStreamFinish(priv->handle) < 0)
> -            g_critical("cannot finish stream");
> -
> -        virStreamFree(priv->handle);
> -    }
> -
>      tmp = priv->sources;
>      while (tmp) {
>          GVirStreamSource *source = tmp->data;
> @@ -218,6 +212,16 @@ static void gvir_stream_finalize(GObject *object)
>          tmp = tmp->next;
>      }
>      g_list_free(priv->sources);
> +    priv->sources = NULL;
> +
> +    if (priv->handle) {
> +        gvir_stream_update_events(self);
> +
> +        if (virStreamFinish(priv->handle) < 0)
> +            g_critical("cannot finish stream");
> +
> +        virStreamFree(priv->handle);
> +    }
>  
>      G_OBJECT_CLASS(gvir_stream_parent_class)->finalize(object);
>  }
> @@ -550,8 +554,8 @@ static void gvir_stream_update_events(GVirStream *stream)
>          } else {
>              virStreamEventAddCallback(priv->handle, mask,
>                                        gvir_stream_handle_events,
> -                                      g_object_ref(stream),
> -                                      g_object_unref);
> +                                      stream,
> +                                      NULL);
>              priv->eventRegistered = TRUE;
>          }
>      } else {
> @@ -600,8 +604,9 @@ static void gvir_stream_source_finalize(GSource *source)
>      GVirStreamPrivate *priv = gsource->stream->priv;
>  
>      priv->sources = g_list_remove(priv->sources, source);
> -
>      gvir_stream_update_events(gsource->stream);
> +
> +    g_clear_object(&gsource->stream);
>  }
>  
>  GSourceFuncs gvir_stream_source_funcs = {
> @@ -657,12 +662,14 @@ guint gvir_stream_add_watch_full(GVirStream *stream,
>                                   gpointer opaque,
>                                   GDestroyNotify notify)
>  {
> +    g_return_val_if_fail(GVIR_IS_STREAM(stream), 0);
> +
>      GVirStreamPrivate *priv = stream->priv;
>      GVirStreamSource *source = (GVirStreamSource*)g_source_new(&gvir_stream_source_funcs,
>                                                                 sizeof(GVirStreamSource));
>      guint ret;
>  
> -    source->stream = stream;
> +    source->stream = g_object_ref(stream);
>      source->cond = cond;
>  
>      if (priority != G_PRIORITY_DEFAULT)

ACK

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|