[libvirt] [PATCHv3] python: Avoid memory leaks on libvirt_virNodeGetMemoryStats

[Eric Blake]

On 02/17/2012 01:15 AM, ajia at redhat.com wrote:
> From: Alex Jia <ajia at redhat.com>
> 
> Detected by valgrind. Leaks are introduced in commit 17c7795.
> 
> * python/libvirt-override.c (libvirt_virNodeGetMemoryStats): fix memory leaks
> and improve codes return value.
> 
> For details, please see the following link:
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=770944
> 
> Signed-off-by: Alex Jia <ajia at redhat.com>
> ---
>  python/libvirt-override.c |   46 ++++++++++++++++++++++++++++++--------------
>  1 files changed, 31 insertions(+), 15 deletions(-)
> 

>  
>      LIBVIRT_BEGIN_ALLOW_THREADS;
>      c_retval = virNodeGetMemoryStats(conn, cellNum, NULL, &nparams, flags);
>      LIBVIRT_END_ALLOW_THREADS;
>      if (c_retval < 0)
> -        return VIR_PY_NONE;
> +        return ret;

Wrong.  At this point, ret is still NULL, but we really do want to
return VIR_PY_NONE (we have a valid libvirt error, and not a python
exception).

>  
>      if (nparams) {
>          if (VIR_ALLOC_N(stats, nparams) < 0)
> -            return VIR_PY_NONE;
> +            return PyErr_NoMemory();

Correct (here we have a python exception, not a libvirt error).

>  
>          LIBVIRT_BEGIN_ALLOW_THREADS;
>          c_retval = virNodeGetMemoryStats(conn, cellNum, stats, &nparams, flags);
>          LIBVIRT_END_ALLOW_THREADS;
> -        if (c_retval < 0) {
> -            VIR_FREE(stats);
> -            return VIR_PY_NONE;
> -        }
> -    }
> -    if (!(ret = PyDict_New())) {
> -        VIR_FREE(stats);
> -        return VIR_PY_NONE;
> +        if (c_retval < 0)
> +            goto error;

Wrong - here's another place where we want to return VIR_PY_NONE.

>      }
> +
> +    if (!(ret = PyDict_New()))
> +        goto error;

Good - here we have a python exception, and ret is still NULL.

> +
>      for (i = 0; i < nparams; i++) {
> -        PyDict_SetItem(ret,
> -                       libvirt_constcharPtrWrap(stats[i].field),
> -                       libvirt_ulonglongWrap(stats[i].value));
> +        key = libvirt_constcharPtrWrap(stats[i].field);
> +        val = libvirt_ulonglongWrap(stats[i].value);
> +
> +        if (!key || !val)
> +            goto error;

Hmm.  Here, ret is allocated, so we mistakenly end up returning a python
object when we really want to return NULL to cover the python error we
just encountered.

> +
> +        if (PyDict_SetItem(ret, key, val) < 0) {
> +            Py_DECREF(ret);
> +            goto error;

Even worse - we are allowing ret to be garbage collected, but still
returning its address.  If PyDict_SetItem fails, you want to return
NULL, not the just-allocated dictionary object.

> +        }
> +
> +        Py_DECREF(key);
> +        Py_DECREF(val);
>      }
>  
>      VIR_FREE(stats);
>      return ret;
> +
> +error:
> +    VIR_FREE(stats);
> +    Py_XDECREF(key);
> +    Py_XDECREF(val);
> +    return ret;

I think we still need a v4 to fix things properly.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120319/6ae04323/attachment-0001.sig>