[Numpy-discussion] Ticket review: #848, leak in PyArray_DescrFromType

[Charles R Harris]

On Tue, Jul 15, 2008 at 9:28 AM, Michael Abbott <michael at araneidae.co.uk>
wrote:

> On Tue, 15 Jul 2008, Michael Abbott wrote:
> > Only half of my patch for this bug has gone into trunk, and without the
> > rest of my patch there remains a leak.
>
> I think I might need to explain a little more about the reason for this
> patch, because obviously the bug it fixes was missed the last time I
> posted on this bug.
>
> So here is the missing part of the patch:
>
> > --- numpy/core/src/scalartypes.inc.src  (revision 5411)
> > +++ numpy/core/src/scalartypes.inc.src  (working copy)
> > @@ -1925,19 +1925,30 @@
> >          goto finish;
> >      }
> >
> > +    Py_XINCREF(typecode);
> >      arr = PyArray_FromAny(obj, typecode, 0, 0, FORCECAST, NULL);
> > -    if ((arr==NULL) || (PyArray_NDIM(arr) > 0)) return arr;
> > +    if ((arr==NULL) || (PyArray_NDIM(arr) > 0)) {
> > +        Py_XDECREF(typecode);
> > +        return arr;
> > +    }
> >      robj = PyArray_Return((PyArrayObject *)arr);
> >
> >  finish:
> > -    if ((robj==NULL) || (robj->ob_type == type)) return robj;
> > +    if ((robj==NULL) || (robj->ob_type == type)) {
> > +        Py_XDECREF(typecode);
> > +        return robj;
> > +    }
> >      /* Need to allocate new type and copy data-area over */
> >      if (type->tp_itemsize) {
> >          itemsize = PyString_GET_SIZE(robj);
> >      }
> >      else itemsize = 0;
> >      obj = type->tp_alloc(type, itemsize);
> > -    if (obj == NULL) {Py_DECREF(robj); return NULL;}
> > +    if (obj == NULL) {
> > +        Py_XDECREF(typecode);
> > +        Py_DECREF(robj);
> > +        return NULL;
> > +    }
> >      if (typecode==NULL)
> >          typecode = PyArray_DescrFromType(PyArray_ at TYPE@);
> >      dest = scalar_value(obj, typecode);
>
> On the face of it it might appear that all the DECREFs are cancelling out
> the first INCREF, but not so.  Let's see two more lines of context:
>
> >      src = scalar_value(robj, typecode);
> >      Py_DECREF(typecode);
>
> Ahah.  That DECREF balances the original PyArray_DescrFromType, or maybe
> the later call ... and of course this has to happen on *ALL* return paths.
> If we now take a closer look at the patch we can see that it's doing two
> separate things:
>
> 1. There's an extra Py_XINCREF to balance the ref count lost to
> PyArray_FromAny and ensure that typecode survives long enough;
>
> 2. Every early return path has an extra Py_XDECREF to balance the creation
> of typecode.
>
> I rest my case for this patch.


Yes, there does look to be a memory leak here. Not to mention a missing NULL
check since PyArray_Scalar not only doesn't swallow a reference, it can't
take a Null value for desc. But the whole function is such a mess I want to
see if we can rewrite it to have a better flow of logic <puts on todo list>

Chuck
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/numpy-discussion/attachments/20080716/1f8b362d/attachment.html>