[libvirt] [libvirt-python PATCH 15/23] all Py*_New function has to be checked for return value

[Pavel Hrdina]

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 at redhat.com>
> > ---
> >  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));

[...]