From c93df6d34eddd3b074ba626b9d1b5accee8c723c Mon Sep 17 00:00:00 2001 From: wlkr73 <86650480+wlkr73@users.noreply.github.com> Date: Sat, 24 Oct 2026 19:28:40 +0000 Subject: [PATCH] gh-160573: Drop the set once when a shared iterator is exhausted setiter_iternext() read si->si_set, checked it for NULL, walked the table under the set's critical section and then cleared si->si_set and decrefed the set after Py_END_CRITICAL_SECTION(). Threads that reach the exhausted branch together all pass the NULL check, so every one of them drops the iterator's single reference and the set is over-released. On a free-threaded debug build the damage shows up at finalization as "refcount is too small" in validate_gc_objects(); when the set is still alive it shows up earlier as a negative refcount the next time sys.getrefcount() reads it. Hold the iterator's own critical section across the size check, the walk, the clear and the counter updates, and store NULL with the same atomic store the lock-free fast path uses for its read. The iterator's reference is then dropped exactly once, after the lock has been released. Add a regression test in test_free_threading: eight threads race to exhaust one iterator over a set with a large dummy tail, and the set's refcount must stay unchanged. Without the fix it aborts with _Py_NegativeRefcount. --- Lib/test/test_free_threading/test_set.py | 49 ++++++++++++ ...-10-24-19-40-00.gh-issue-160573.sT9kQe.rst | 4 + Objects/setobject.c | 75 ++++++++++++------- 3 files changed, 102 insertions(+), 26 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-10-24-19-40-00.gh-issue-160573.sT9kQe.rst diff --git a/Lib/test/test_free_threading/test_set.py b/Lib/test/test_free_threading/test_set.py index 9dd3d68d5dad13..7c9ffa53cfbee0 100644 --- a/Lib/test/test_free_threading/test_set.py +++ b/Lib/test/test_free_threading/test_set.py @@ -1,3 +1,4 @@ +import sys import unittest from threading import Thread, Barrier, Event from test.support import threading_helper @@ -159,5 +160,53 @@ class LargeSetTest(RaceTestBase, unittest.TestCase): SET_SIZE = 20 # larger than PySet_MINSIZE +class TestSetIteratorExhaustion(unittest.TestCase): + """Exhausting one set iterator from several threads at once.""" + + # One live element followed by a long run of dummy entries widens the + # window between reading si_set and clearing it, so threads that arrive + # together all reach the exhausted branch. + SET_SIZE = 300_000 + NUM_THREADS = 8 + ROUNDS = 3 + + @threading_helper.requires_working_threading() + def test_racing_exhaustion_releases_set_once(self): + s = set(range(self.SET_SIZE)) + for i in range(1, self.SET_SIZE): + s.discard(i) + # Extra references keep a wrongly released set alive, so every lost + # reference shows up as a refcount difference instead of a crash. + keep = [s] * 1000 + base = sys.getrefcount(s) + try: + for _ in range(self.ROUNDS): + it = iter(s) + next(it) # leave the iterator on the exhausted branch + barrier = Barrier(self.NUM_THREADS, timeout=10) + errors = [] + + def exhaust(): + barrier.wait() + try: + next(it) + except StopIteration: + pass + except BaseException as exc: + errors.append(exc) + + threads = [Thread(target=exhaust) + for _ in range(self.NUM_THREADS)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + self.assertEqual(errors, []) + self.assertEqual(sys.getrefcount(s), base) + finally: + del keep + + if __name__ == "__main__": unittest.main() diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-10-24-19-40-00.gh-issue-160573.sT9kQe.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-10-24-19-40-00.gh-issue-160573.sT9kQe.rst new file mode 100644 index 00000000000000..0c4f49ca1ba5bb --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-10-24-19-40-00.gh-issue-160573.sT9kQe.rst @@ -0,0 +1,4 @@ +Fix a crash when a :class:`set` iterator that is shared between threads reaches +its end in a free-threaded build: every thread that walked off the end of the +table released the iterator's reference to the set, so the set was over-released. +The reference is now dropped once, under the iterator's critical section. diff --git a/Objects/setobject.c b/Objects/setobject.c index a762edf5933e18..0ae20f83f112a0 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -1093,42 +1093,65 @@ static PyObject *setiter_iternext(PyObject *self) { setiterobject *si = (setiterobject*)self; PyObject *key = NULL; - Py_ssize_t i, mask; + Py_ssize_t i = 0, mask; setentry *entry; - PySetObject *so = si->si_set; + PySetObject *so; + int released = 0; + int size_changed = 0; - if (so == NULL) + /* The fast path still reads si_set without the lock, so it must be the + atomic load; a plain read races with the clearing thread's store. */ + if (FT_ATOMIC_LOAD_PTR_RELAXED(si->si_set) == NULL) return NULL; - assert (PyAnySet_Check(so)); - Py_ssize_t so_used = FT_ATOMIC_LOAD_SSIZE_RELAXED(so->used); - Py_ssize_t si_used = FT_ATOMIC_LOAD_SSIZE_RELAXED(si->si_used); - if (si_used != so_used) { + /* si_set is cleared by whichever thread walks off the end of the table + first, so both the read and the clear need the iterator's own lock: an + unlocked read can pick up a pointer to a set that the release has + already deallocated. */ + Py_BEGIN_CRITICAL_SECTION(self); + so = si->si_set; + if (so != NULL) { + assert (PyAnySet_Check(so)); + + Py_ssize_t so_used = FT_ATOMIC_LOAD_SSIZE_RELAXED(so->used); + Py_ssize_t si_used = FT_ATOMIC_LOAD_SSIZE_RELAXED(si->si_used); + if (si_used != so_used) { + si->si_used = -1; /* Make this state sticky */ + size_changed = 1; + } + else { + Py_BEGIN_CRITICAL_SECTION(so); + i = si->si_pos; + assert(i>=0); + entry = so->table; + mask = so->mask; + while (i <= mask && (entry[i].key == NULL || entry[i].key == dummy)) { + i++; + } + if (i <= mask) { + key = Py_NewRef(entry[i].key); + } + Py_END_CRITICAL_SECTION(); + si->si_pos = i+1; + if (key == NULL) { + FT_ATOMIC_STORE_PTR_RELAXED(si->si_set, NULL); + released = 1; + } + else { + si->len--; + } + } + } + Py_END_CRITICAL_SECTION(); + + if (size_changed) { PyErr_SetString(PyExc_RuntimeError, "Set changed size during iteration"); - si->si_used = -1; /* Make this state sticky */ return NULL; } - - Py_BEGIN_CRITICAL_SECTION(so); - i = si->si_pos; - assert(i>=0); - entry = so->table; - mask = so->mask; - while (i <= mask && (entry[i].key == NULL || entry[i].key == dummy)) { - i++; - } - if (i <= mask) { - key = Py_NewRef(entry[i].key); - } - Py_END_CRITICAL_SECTION(); - si->si_pos = i+1; - if (key == NULL) { - si->si_set = NULL; + if (released) { Py_DECREF(so); - return NULL; } - si->len--; return key; }