gh-154130: Fix double decref of the dict in shared dict iterators

In the free-threaded build, threads advancing the same dict iterator each
read `di_dict` as a borrowed reference, so concurrently exhausting it could run
`di->di_dict = NULL; Py_DECREF(d)` more than once and free the dict while
other threads were still using it.  Reversed iterators were also affected:
they dropped the reference while holding the dict's critical section.

In the free-threaded build, keep the iterator's reference to the dict until
the iterator is deallocated (as list and tuple iterators do) and mark
exhaustion by setting `di_pos` to -1.

Author: mpage

Lib/test/test_free_threading/test_dict.py

@@ -493,5 +493,42 @@ def race(iter_times: int,
         CHURN_SIZE = 512
         race(ITER_TIMES, CHURN_SIZE, APPENDER_THREADS, REPLACER_THREADS)
 
+    def test_racing_shared_iterator_exhaustion(self):
+        # gh-154130: exhausting a dict iterator shared between threads must
+        # not release the iterator's reference to the dict more than once.
+        makers = [
+            lambda d: iter(d),
+            lambda d: iter(d.values()),
+            lambda d: iter(d.items()),
+            lambda d: reversed(d),
+            lambda d: reversed(d.items()),
+        ]
+        for make in makers:
+            cell = [make({i: i for i in range(8)})]
+
+            def worker():
+                for _ in range(5000):
+                    it = cell[0]
+                    try:
+                        next(it)
+                    except StopIteration:
+                        cell[0] = make({i: i for i in range(8)})
+                    except RuntimeError:
+                        # Concurrent next() calls may observe each other's
+                        # updates as a modification; only crashes are bugs.
+                        pass
+
+            with self.subTest(make=make):
+                threading_helper.run_concurrently(worker, nthreads=4)
+
+    def test_exhausted_iterator_stays_exhausted(self):
+        d = {1: 1}
+        for it in (iter(d), iter(d.items()), reversed(d)):
+            self.assertEqual(len(list(it)), 1)
+            d[2] = 2
+            self.assertRaises(StopIteration, next, it)
+            self.assertEqual(it.__length_hint__(), 0)
+            del d[2]
+
 if __name__ == "__main__":
     unittest.main()

Objects/dictobject.c

@@ -5705,6 +5705,23 @@ static PyMethodDef dictiter_methods[] = {
     {NULL,              NULL}           /* sentinel */
 };
 
+/* Mark the iterator as exhausted.  In the free-threaded build an iterator
+   may be advanced by several threads at once, each holding a borrowed
+   pointer to di_dict, so the iterator keeps its reference to the dict until
+   it is deallocated (as list and tuple iterators do) and records exhaustion
+   by setting di_pos to -1. */
+static void
+dictiter_set_exhausted(dictiterobject *di, PyDictObject *d)
+{
+#ifdef Py_GIL_DISABLED
+    _Py_atomic_store_ssize_relaxed(&di->di_pos, -1);
+    _Py_atomic_store_ssize_relaxed(&di->len, 0);
+#else
+    di->di_dict = NULL;
+    Py_DECREF(d);
+#endif
+}
+
 #ifdef Py_GIL_DISABLED
 
 static int
@@ -5970,16 +5987,18 @@ dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self,
     assert (PyAnyDict_Check(d));
     ASSERT_DICT_LOCKED(d);
 
+    i = FT_ATOMIC_LOAD_SSIZE_RELAXED(di->di_pos);
+    if (i < 0) {
+        return -1;  /* exhausted */
+    }
+
     if (di->di_used != d->ma_used) {
         PyErr_SetString(PyExc_RuntimeError,
                         "dictionary changed size during iteration");
         di->di_used = -1; /* Make this state sticky */
         return -1;
     }
 
-    i = FT_ATOMIC_LOAD_SSIZE_RELAXED(di->di_pos);
-
-    assert(i >= 0);
     if (_PyDict_HasSplitTable(d)) {
         if (i >= d->ma_used)
             goto fail;
@@ -6030,8 +6049,7 @@ dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self,
     return 0;
 
 fail:
-    di->di_dict = NULL;
-    Py_DECREF(d);
+    dictiter_set_exhausted(di, d);
     return -1;
 }
 
@@ -6075,6 +6093,11 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self,
 
     assert (PyAnyDict_Check(d));
 
+    i = _Py_atomic_load_ssize_relaxed(&di->di_pos);
+    if (i < 0) {
+        return -1;  /* exhausted */
+    }
+
     if (di->di_used != _Py_atomic_load_ssize_relaxed(&d->ma_used)) {
         PyErr_SetString(PyExc_RuntimeError,
                         "dictionary changed size during iteration");
@@ -6084,9 +6107,7 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self,
 
     ensure_shared_on_read(d);
 
-    i = _Py_atomic_load_ssize_relaxed(&di->di_pos);
     k = _Py_atomic_load_ptr_acquire(&d->ma_keys);
-    assert(i >= 0);
     if (_PyDict_HasSplitTable(d)) {
         PyDictValues *values = _Py_atomic_load_ptr_consume(&d->ma_values);
         if (values == NULL) {
@@ -6165,8 +6186,7 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self,
                     "dictionary keys changed during iteration");
 
 fail:
-    di->di_dict = NULL;
-    Py_DECREF(d);
+    dictiter_set_exhausted(di, d);
     return -1;
 
 try_locked:
@@ -6354,8 +6374,7 @@ dictreviter_iter_lock_held(PyDictObject *d, PyObject *self)
     }
 
 fail:
-    di->di_dict = NULL;
-    Py_DECREF(d);
+    dictiter_set_exhausted(di, d);
     return NULL;
 }
 
@@ -6367,6 +6386,11 @@ dictreviter_iternext(PyObject *self)
 
     if (d == NULL)
         return NULL;
+#ifdef Py_GIL_DISABLED
+    if (_Py_atomic_load_ssize_relaxed(&di->di_pos) < 0) {
+        return NULL;  /* exhausted */
+    }
+#endif
 
     PyObject *value;
     Py_BEGIN_CRITICAL_SECTION(d);