Merge pull request #13871 from seberg/ugly-refcount-changing

MAINT: Ensure array_dealloc does not modify refcount of self

6 files changed, 49 insertions, 37 deletions

diff --git a/numpy/core/src/multiarray/arrayobject.c b/numpy/core/src/multiarray/arrayobject.c
index bbb736fd0..eb939f47c 100644
--- a/numpy/core/src/multiarray/arrayobject.c
+++ b/numpy/core/src/multiarray/arrayobject.c
@@ -483,10 +483,11 @@ array_dealloc(PyArrayObject *self)
             char const * msg = "WRITEBACKIFCOPY detected in array_dealloc. "
                 " Required call to PyArray_ResolveWritebackIfCopy or "
                 "PyArray_DiscardWritebackIfCopy is missing.";
-            Py_INCREF(self); /* hold on to self in next call  since if
-                              * refcount == 0 it will recurse back into
-                              *array_dealloc
-                              */
+            /*
+             * prevent reaching 0 twice and thus recursing into dealloc.
+             * Increasing sys.gettotalrefcount, but path should not be taken.
+             */
+            Py_INCREF(self);
             WARN_IN_DEALLOC(PyExc_RuntimeWarning, msg);
             retval = PyArray_ResolveWritebackIfCopy(self);
             if (retval < 0)
@@ -500,10 +501,11 @@ array_dealloc(PyArrayObject *self)
             char const * msg = "UPDATEIFCOPY detected in array_dealloc. "
                 " Required call to PyArray_ResolveWritebackIfCopy or "
                 "PyArray_DiscardWritebackIfCopy is missing";
-            Py_INCREF(self); /* hold on to self in next call  since if
-                              * refcount == 0 it will recurse back into
-                              *array_dealloc
-                              */
+            /*
+             * prevent reaching 0 twice and thus recursing into dealloc.
+             * Increasing sys.gettotalrefcount, but path should not be taken.
+             */
+            Py_INCREF(self);
             /* 2017-Nov-10 1.14 */
             WARN_IN_DEALLOC(PyExc_DeprecationWarning, msg);
             retval = PyArray_ResolveWritebackIfCopy(self);
@@ -523,12 +525,7 @@ array_dealloc(PyArrayObject *self)
     if ((fa->flags & NPY_ARRAY_OWNDATA) && fa->data) {
         /* Free internal references if an Object array */
         if (PyDataType_FLAGCHK(fa->descr, NPY_ITEM_REFCOUNT)) {
-            Py_INCREF(self); /*hold on to self */
             PyArray_XDECREF(self);
-            /*
-             * Don't need to DECREF -- because we are deleting
-             * self already...
-             */
         }
         npy_free_cache(fa->data, PyArray_NBYTES(self));
     }
diff --git a/numpy/core/src/multiarray/descriptor.c b/numpy/core/src/multiarray/descriptor.c
index 4d22c9ee7..c7db092e6 100644
--- a/numpy/core/src/multiarray/descriptor.c
+++ b/numpy/core/src/multiarray/descriptor.c
@@ -102,6 +102,7 @@ _arraydescr_from_dtype_attr(PyObject *obj, PyArray_Descr **newdescr)
     if (Py_EnterRecursiveCall(
             " while trying to convert the given data type from its "
             "`.dtype` attribute.") != 0) {
+        Py_DECREF(dtypedescr);
         return 1;
     }
 
diff --git a/numpy/core/src/multiarray/iterators.c b/numpy/core/src/multiarray/iterators.c
index 83eafaf74..0d7679fe7 100644
--- a/numpy/core/src/multiarray/iterators.c
+++ b/numpy/core/src/multiarray/iterators.c
@@ -116,10 +116,12 @@ get_ptr_simple(PyArrayIterObject* iter, npy_intp *coordinates)
  * This is common initialization code between PyArrayIterObject and
  * PyArrayNeighborhoodIterObject
  *
- * Increase ao refcount
+ * Steals a reference to the array object which gets removed at deallocation,
+ * if the iterator is allocated statically and its dealloc not called, it
+ * can be thought of as borrowing the reference.
  */
-static PyObject *
-array_iter_base_init(PyArrayIterObject *it, PyArrayObject *ao)
+NPY_NO_EXPORT void
+PyArray_RawIterBaseInit(PyArrayIterObject *it, PyArrayObject *ao)
 {
     int nd, i;
 
@@ -131,7 +133,6 @@ array_iter_base_init(PyArrayIterObject *it, PyArrayObject *ao)
     else {
         it->contiguous = 0;
     }
-    Py_INCREF(ao);
     it->ao = ao;
     it->size = PyArray_SIZE(ao);
     it->nd_m1 = nd - 1;
@@ -155,7 +156,7 @@ array_iter_base_init(PyArrayIterObject *it, PyArrayObject *ao)
     it->translate = &get_ptr_simple;
     PyArray_ITER_RESET(it);
 
-    return (PyObject *)it;
+    return;
 }
 
 static void
@@ -170,6 +171,10 @@ array_iter_base_dealloc(PyArrayIterObject *it)
 NPY_NO_EXPORT PyObject *
 PyArray_IterNew(PyObject *obj)
 {
+    /*
+     * Note that internall PyArray_RawIterBaseInit may be called directly on a
+     * statically allocated PyArrayIterObject.
+     */
     PyArrayIterObject *it;
     PyArrayObject *ao;
 
@@ -186,7 +191,8 @@ PyArray_IterNew(PyObject *obj)
         return NULL;
     }
 
-    array_iter_base_init(it, ao);
+    Py_INCREF(ao);  /* PyArray_RawIterBaseInit steals a reference */
+    PyArray_RawIterBaseInit(it, ao);
     return (PyObject *)it;
 }
 
@@ -390,6 +396,10 @@ arrayiter_next(PyArrayIterObject *it)
 static void
 arrayiter_dealloc(PyArrayIterObject *it)
 {
+    /*
+     * Note that it is possible to statically allocate a PyArrayIterObject,
+     * which does not call this function.
+     */
     array_iter_base_dealloc(it);
     PyArray_free(it);
 }
@@ -1779,7 +1789,8 @@ PyArray_NeighborhoodIterNew(PyArrayIterObject *x, npy_intp *bounds,
     }
     PyObject_Init((PyObject *)ret, &PyArrayNeighborhoodIter_Type);
 
-    array_iter_base_init((PyArrayIterObject*)ret, x->ao);
+    Py_INCREF(x->ao);  /* PyArray_RawIterBaseInit steals a reference */
+    PyArray_RawIterBaseInit((PyArrayIterObject*)ret, x->ao);
     Py_INCREF(x);
     ret->_internal_iter = x;
 
diff --git a/numpy/core/src/multiarray/iterators.h b/numpy/core/src/multiarray/iterators.h
index 376dc154a..d942f45b8 100644
--- a/numpy/core/src/multiarray/iterators.h
+++ b/numpy/core/src/multiarray/iterators.h
@@ -7,4 +7,7 @@ NPY_NO_EXPORT PyObject
 NPY_NO_EXPORT int
 iter_ass_subscript(PyArrayIterObject *, PyObject *, PyObject *);
 
+NPY_NO_EXPORT void
+PyArray_RawIterBaseInit(PyArrayIterObject *it, PyArrayObject *ao);
+
 #endif
diff --git a/numpy/core/src/multiarray/refcount.c b/numpy/core/src/multiarray/refcount.c
index b8230c81a..6033929d9 100644
--- a/numpy/core/src/multiarray/refcount.c
+++ b/numpy/core/src/multiarray/refcount.c
@@ -11,6 +11,7 @@
 #define _MULTIARRAYMODULE
 #include "numpy/arrayobject.h"
 #include "numpy/arrayscalars.h"
+#include "iterators.h"
 
 #include "npy_config.h"
 
@@ -210,21 +211,22 @@ PyArray_XDECREF(PyArrayObject *mp)
     npy_intp i, n;
     PyObject **data;
     PyObject *temp;
-    PyArrayIterObject *it;
+    /*
+     * statically allocating it allows this function to not modify the
+     * reference count of the array for use during dealloc.
+     * (statically is not necessary as such)
+     */
+    PyArrayIterObject it;
 
     if (!PyDataType_REFCHK(PyArray_DESCR(mp))) {
         return 0;
     }
     if (PyArray_DESCR(mp)->type_num != NPY_OBJECT) {
-        it = (PyArrayIterObject *)PyArray_IterNew((PyObject *)mp);
-        if (it == NULL) {
-            return -1;
+        PyArray_RawIterBaseInit(&it, mp);
+        while(it.index < it.size) {
+            PyArray_Item_XDECREF(it.dataptr, PyArray_DESCR(mp));
+            PyArray_ITER_NEXT(&it);
         }
-        while(it->index < it->size) {
-            PyArray_Item_XDECREF(it->dataptr, PyArray_DESCR(mp));
-            PyArray_ITER_NEXT(it);
-        }
-        Py_DECREF(it);
         return 0;
     }
 
@@ -242,16 +244,12 @@ PyArray_XDECREF(PyArrayObject *mp)
         }
     }
     else { /* handles misaligned data too */
-        it = (PyArrayIterObject *)PyArray_IterNew((PyObject *)mp);
-        if (it == NULL) {
-            return -1;
-        }
-        while(it->index < it->size) {
-            NPY_COPY_PYOBJECT_PTR(&temp, it->dataptr);
+        PyArray_RawIterBaseInit(&it, mp);
+        while(it.index < it.size) {
+            NPY_COPY_PYOBJECT_PTR(&temp, it.dataptr);
             Py_XDECREF(temp);
-            PyArray_ITER_NEXT(it);
+            PyArray_ITER_NEXT(&it);
         }
-        Py_DECREF(it);
     }
     return 0;
 }
diff --git a/numpy/core/tests/test_multiarray.py b/numpy/core/tests/test_multiarray.py
index 53e538f7d..bfc25e9ee 100644
--- a/numpy/core/tests/test_multiarray.py
+++ b/numpy/core/tests/test_multiarray.py
@@ -8080,6 +8080,8 @@ class TestWritebackIfCopy(object):
         arr_wb[...] = 100
         assert_equal(arr, -100)
 
+    @pytest.mark.leaks_references(
+            reason="increments self in dealloc; ignore since deprecated path.")
     def test_dealloc_warning(self):
         with suppress_warnings() as sup:
             sup.record(RuntimeWarning)