On 02/17/2012 01:15 AM, ajia@xxxxxxxxxx wrote: > From: Alex Jia <ajia@xxxxxxxxxx> > > 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@xxxxxxxxxx> > --- > 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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list