On Tue, Jul 15, 2008 at 9:28 AM, Michael Abbott <michael@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_@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