BUG: MapIter leaked itself and arr and index objects

MapIterBind incorrectly behaved when failing, cleaning up most
but not everything. The callers have to decref the mapiter
object. Which basically means that MapIterBind does not even
have to decref the new fields (they are decrefed in the
mapiter dealloc anyway).

Note that trying to add tests checking the refcounting
failed, but the tests seem to fail at least in many cases
not because of the indexing.

Closes gh-3589

2 files changed, 48 insertions, 19 deletions

diff --git a/numpy/core/src/multiarray/mapping.c b/numpy/core/src/multiarray/mapping.c
index 25f3dad9b..445edb0f2 100644
--- a/numpy/core/src/multiarray/mapping.c
+++ b/numpy/core/src/multiarray/mapping.c
@@ -1009,7 +1009,8 @@ array_subscript_fancy(PyArrayObject *self, PyObject *op, int fancy)
         Py_DECREF(mit);
         return rval;
     }
-    if (PyArray_MapIterBind(mit, self) != 0) { 
+    if (PyArray_MapIterBind(mit, self) != 0) {
+        Py_DECREF(mit);
         return NULL;
     }
     other = (PyObject *)PyArray_GetMap(mit);
@@ -1321,6 +1322,7 @@ array_ass_sub_fancy(PyArrayObject *self, PyObject *ind, PyObject *op, int fancy)
         return rval;
     }
     if (PyArray_MapIterBind(mit, self) != 0) {
+        Py_DECREF(mit);
         return -1;
     }
     ret = PyArray_SetMap(mit, op);
@@ -1776,12 +1778,12 @@ PyArray_MapIterBind(PyArrayMapIterObject *mit, PyArrayObject *arr)
     if (subnd < 0) {
         PyErr_SetString(PyExc_IndexError,
                         "too many indices for array");
-        goto fail;
+        return -1;
     }
 
     mit->ait = (PyArrayIterObject *)PyArray_IterNew((PyObject *)arr);
     if (mit->ait == NULL) {
-        goto fail;
+        return -1;
     }
 
     /*
@@ -1800,14 +1802,14 @@ PyArray_MapIterBind(PyArrayMapIterObject *mit, PyArrayObject *arr)
         Py_INCREF(arr);
         obj = PyArray_EnsureArray((PyObject *)arr);
         if (obj == NULL) {
-            goto fail;
+            return -1;
         }
         sub = array_subscript_simple((PyArrayObject *)obj, mit->indexobj, 0);
         Py_DECREF(obj);
     }
 
     if (sub == NULL) {
-        goto fail;
+        return -1;
     }
 
     subnd = PyArray_NDIM(sub);
@@ -1824,7 +1826,7 @@ PyArray_MapIterBind(PyArrayMapIterObject *mit, PyArrayObject *arr)
     mit->subspace = (PyArrayIterObject *)PyArray_IterNew(sub);
     Py_DECREF(sub);
     if (mit->subspace == NULL) {
-        goto fail;
+        return -1;
     }
 
     if (mit->nd + subnd > NPY_MAXDIMS) {
@@ -1832,7 +1834,7 @@ PyArray_MapIterBind(PyArrayMapIterObject *mit, PyArrayObject *arr)
                      "number of dimensions must be within [0, %d], "
                      "indexed array has %d",
                      NPY_MAXDIMS, mit->nd + subnd);
-        goto fail;
+        return -1;
     }
 
     /* Expand dimensions of result */
@@ -1895,7 +1897,7 @@ PyArray_MapIterBind(PyArrayMapIterObject *mit, PyArrayObject *arr)
                              "unexpected object "       \
                              "(%s) in selection position %d",
                              Py_TYPE(obj)->tp_name, i);
-                goto fail;
+                return -1;
             }
             else {
                 mit->bscoord[curraxis] = start;
@@ -1913,12 +1915,12 @@ PyArray_MapIterBind(PyArrayMapIterObject *mit, PyArrayObject *arr)
     if (mit->size < 0) {
         PyErr_SetString(PyExc_ValueError,
                         "dimensions too large in fancy indexing");
-        goto fail;
+        return -1;
     }
     if (mit->ait->size == 0 && mit->size != 0) {
         PyErr_SetString(PyExc_IndexError,
                         "invalid index into a 0-size array");
-        goto fail;
+        return -1;
     }
 
     for (i = 0; i < mit->numiter; i++) {
@@ -1930,20 +1932,13 @@ PyArray_MapIterBind(PyArrayMapIterObject *mit, PyArrayObject *arr)
             indptr = ((npy_intp *)it->dataptr);
             indval = *indptr;
             if (check_and_adjust_index(&indval, dimsize, mit->iteraxes[i]) < 0) {
-                goto fail;
+                return -1;
             }
             PyArray_ITER_NEXT(it);
         }
         PyArray_ITER_RESET(it);
     }
     return 0;
-
- fail:
-    Py_XDECREF(mit->subspace);
-    Py_XDECREF(mit->ait);
-    mit->subspace = NULL;
-    mit->ait = NULL;
-    return -1;
 }
 
 
@@ -2137,6 +2132,7 @@ PyArray_MapIterArray(PyArrayObject * a, PyObject * index)
     }
 
     if (PyArray_MapIterBind(mit, a) != 0) {
+        Py_DECREF(mit);
         return NULL;
     }
     PyArray_MapIterReset(mit);
diff --git a/numpy/core/tests/test_indexing.py b/numpy/core/tests/test_indexing.py
index 3c74bc466..f8105ecc3 100644
--- a/numpy/core/tests/test_indexing.py
+++ b/numpy/core/tests/test_indexing.py
@@ -4,7 +4,7 @@ import numpy as np
 from itertools import product
 from numpy.compat import asbytes
 from numpy.testing import *
-import sys, warnings
+import sys, warnings, gc
 
 
 class TestIndexing(TestCase):
@@ -406,8 +406,10 @@ class TestMultiIndexingAutomated(TestCase):
         try:
             mimic_get, no_copy = self._get_multi_index(arr, index)
         except Exception as e:
+            prev_refcount = sys.getrefcount(arr)
             assert_raises(Exception, arr.__getitem__, index)
             assert_raises(Exception, arr.__setitem__, index, 0)
+            assert_equal(prev_refcount, sys.getrefcount(arr))
             return
 
         self._compare_index_result(arr, index, mimic_get, no_copy)
@@ -427,8 +429,10 @@ class TestMultiIndexingAutomated(TestCase):
         try:
             mimic_get, no_copy = self._get_multi_index(arr, (index,))
         except Exception as e:
+            prev_refcount = sys.getrefcount(arr)
             assert_raises(Exception, arr.__getitem__, index)
             assert_raises(Exception, arr.__setitem__, index, 0)
+            assert_equal(prev_refcount, sys.getrefcount(arr))
             return
 
         self._compare_index_result(arr, index, mimic_get, no_copy)
@@ -484,6 +488,14 @@ class TestMultiIndexingAutomated(TestCase):
     def test_multidim(self):
         # Automatically test combinations with complex indexes on 2nd (or 1st)
         # spot and the simple ones in one other spot.
+
+        # These refcount check fails, however the error seems not the indexing
+        ## Store refcount of the indexing objects, to make sure we don't leak.
+        #gc.collect()
+        #complex_refs_old = [sys.getrefcount(_) for _ in self.complex_indices]
+        #simple_refs_old = [sys.getrefcount(_) for _ in self.simple_indices]
+        #fill_refs_old = [sys.getrefcount(_) for _ in self.fill_indices]
+
         with warnings.catch_warnings():
             # This is so that np.array(True) is not accepted in a full integer
             # index, when running the file seperatly.
@@ -496,6 +508,17 @@ class TestMultiIndexingAutomated(TestCase):
                     index = tuple(i for i in index if i != 'skip')
                     self._check_multi_index(self.a, index)
                     self._check_multi_index(self.b, index)
+
+        ## Test that none of the indexing objects leaked for any of the many
+        ## different tries (testing after every single one seems overly complex)
+        #complex_refs_new = [sys.getrefcount(_) for _ in self.complex_indices]
+        #simple_refs_new = [sys.getrefcount(_) for _ in self.simple_indices]
+        #fill_refs_new = [sys.getrefcount(_) for _ in self.fill_indices]
+        #gc.collect()
+        #assert_equal(complex_refs_new, complex_refs_old)
+        #assert_equal(simple_refs_new, simple_refs_old)
+        #assert_equal(fill_refs_new, fill_refs_old)
+
         # Check very simple item getting:
         self._check_multi_index(self.a, (0,0,0,0))
         self._check_multi_index(self.b, (0,0,0,0))
@@ -508,11 +531,21 @@ class TestMultiIndexingAutomated(TestCase):
 
     def test_1d(self):
         a = np.arange(10)
+
+        # These refcount check fails, however the error seems not the indexing
+        ## Store refcount of the indexing objects, to make sure we don't leak.
+        #gc.collect()
+        #complex_refs_old = [sys.getrefcount(_) for _ in self.complex_indices]
+
         with warnings.catch_warnings():
             warnings.filterwarnings('error', '', DeprecationWarning)
             for index in self.complex_indices:
                 self._check_single_index(a, index)
 
+        #gc.collect()
+        #complex_refs_new = [sys.getrefcount(_) for _ in self.complex_indices]
+        #assert_equal(complex_refs_new, complex_refs_old)
+
 
 if __name__ == "__main__":
     run_module_suite()