On Sat, Sep 26, 2015 at 09:37:23AM -0400, John Ferlan wrote:
>
> $subj:
>
> Must check return value for all Py*_New functions
Thanks, I'll update it.
>
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > If the function fails, we need to cleanup memory and return NULL.
> >
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> > libvirt-lxc-override.c | 10 +-
> > libvirt-override.c | 269 +++++++++++++++++++++++++++++++++----------------
> > 2 files changed, 190 insertions(+), 89 deletions(-)
> >
>
> General comment... 4 patches ago we were returning PyErr_NoMemory on
> allocation failures - that would seem to be the case for these too,
> right? Or is this "just different" enough that it would be necessary?
No, because all python APIs sets an exception in case of error.
>
> > diff --git a/libvirt-lxc-override.c b/libvirt-lxc-override.c
> > index 20d1cf4..b0550c7 100644
> > --- a/libvirt-lxc-override.c
> > +++ b/libvirt-lxc-override.c
> > @@ -79,7 +79,9 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self ATTRIBUTE_UNUSED,
> > if (c_retval < 0)
> > return VIR_PY_NONE;
> >
> > - py_retval = PyList_New(0);
> > + if ((py_retval = PyList_New(0)) == NULL)
> > + goto error;
> > +
> > for (i = 0; i < c_retval; i++) {
> > PyObject *item = NULL;
> >
> > @@ -91,6 +93,8 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self ATTRIBUTE_UNUSED,
> > goto error;
> > }
> > }
> > +
> > + cleanup:
> > VIR_FREE(fdlist);
> > return py_retval;
> >
> > @@ -98,8 +102,8 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self ATTRIBUTE_UNUSED,
> > for (i = 0; i < c_retval; i++) {
> > VIR_FORCE_CLOSE(fdlist[i]);
> > }
> > - VIR_FREE(fdlist);
> > - return NULL;
> > + Py_CLEAR(py_retval);
> > + goto cleanup;
> > }
> > /************************************************************************
> > * *
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index 0df6844..92c31b8 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -1859,7 +1859,7 @@ static void
> > libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void *ctx,
> > virErrorPtr err)
> > {
> > - PyObject *list, *info;
> > + PyObject *list = NULL, *info = NULL;
> > PyObject *result;
> >
> > DEBUG("libvirt_virErrorFuncHandler(%p, %s, ...) called\n", ctx,
> > @@ -1874,8 +1874,12 @@ libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void *ctx,
> > (libvirt_virPythonErrorFuncHandler == Py_None)) {
> > virDefaultErrorFunc(err);
> > } else {
> > - list = PyTuple_New(2);
> > - info = PyTuple_New(9);
> > + if ((list = PyTuple_New(2)) == NULL)
> > + goto cleanup;
> > +
> > + if ((info = PyTuple_New(9)) == NULL)
> > + goto cleanup;
> > +
> > PyTuple_SetItem(list, 0, libvirt_virPythonErrorFuncCtxt);
> > PyTuple_SetItem(list, 1, info);
> > Py_XINCREF(libvirt_virPythonErrorFuncCtxt);
> > @@ -1894,6 +1898,10 @@ libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void *ctx,
> > Py_XDECREF(result);
> > }
> >
> > + cleanup:
> > + Py_XDECREF(list);
> > + Py_XDECREF(info);
> > +
> > LIBVIRT_RELEASE_THREAD_STATE;
> > }
> >
> > @@ -1938,12 +1946,12 @@ virConnectCredCallbackWrapper(virConnectCredentialPtr cred,
> > unsigned int ncred,
> > void *cbdata)
> > {
> > - PyObject *list;
> > + PyObject *list = NULL;
> > PyObject *pycred;
> > PyObject *pyauth = (PyObject *)cbdata;
> > PyObject *pycbdata;
> > PyObject *pycb;
> > - PyObject *pyret;
> > + PyObject *pyret = NULL;
> > int ret = -1;
> > size_t i;
> >
> > @@ -1952,11 +1960,17 @@ virConnectCredCallbackWrapper(virConnectCredentialPtr cred,
> > pycb = PyList_GetItem(pyauth, 1);
> > pycbdata = PyList_GetItem(pyauth, 2);
> >
> > - list = PyTuple_New(2);
> > - pycred = PyTuple_New(ncred);
> > + if((list = PyTuple_New(2)) == NULL)
>
> if ((
Nice catch, thanks.
>
> > + goto cleanup;
> > +
> > + if ((pycred = PyTuple_New(ncred)) == NULL)
> > + goto cleanup;
> > +
> > for (i = 0; i < ncred; i++) {
> > - PyObject *pycreditem;
> > - pycreditem = PyList_New(5);
> > + PyObject *pycreditem = PyList_New(5);
> > + if (pycreditem == NULL)
> > + goto cleanup;
> > +
>
> nit: could all be one line (any typos are mine ;-))
>
> if ((pycreditem = PyList_New(5)) == NULL)
>
> Nothing jumped out at me in the rest - although I admit my eyes & brain
> are starting to tire ;-)
It's a lot of almost same changes, easy to miss something. I'll update that,
thanks.
Pavel
>
> John
> > PyTuple_SetItem(pycred, i, pycreditem);
> > PyList_SetItem(pycreditem, 0, libvirt_intWrap((long) cred[i].type));
> > PyList_SetItem(pycreditem, 1, libvirt_constcharPtrWrap(cred[i].prompt));
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list