Re: [libvirt] [PATCHv3] python: Avoid memory leaks on libvirt_virNodeGetMemoryStats
On 02/17/2012 01:15 AM, ajia(a)redhat.com wrote:
From: Alex Jia <ajia(a)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(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)