From b5e17e1c29628fb261ca1a0f566e364e7fefcd94 Mon Sep 17 00:00:00 2001 From: Bhuvansh Kataria Date: Sat, 15 Aug 2026 20:41:05 +0000 Subject: [PATCH 1/4] gh-153928: make Unicode iterators thread-safe --- .../test_free_threading/test_iteration.py | 43 +++++++++ Objects/unicodeobject.c | 88 +++++++++++++++---- 2 files changed, 114 insertions(+), 17 deletions(-) diff --git a/Lib/test/test_free_threading/test_iteration.py b/Lib/test/test_free_threading/test_iteration.py index 44d3e9ccfdd14e..f2df0b4acb9f8f 100644 --- a/Lib/test/test_free_threading/test_iteration.py +++ b/Lib/test/test_free_threading/test_iteration.py @@ -125,3 +125,46 @@ def assert_iterator_results(self, results, expected): extra_items = set(results) - set(expected) for item in extra_items: self.assertEqual((item - expected.start) % expected.step, 0) + +class ContendedStringIterationTest(ContendedTupleIterationTest): + def make_testdata(self, n): + return "A" * n + + def test_shared_iterator(self): + """Test iteration over a shared string iterator""" + seq = self.make_testdata(NUMITEMS) + it = iter(seq) + results = [] + start = threading.Barrier(NUMTHREADS) + + def worker(): + items = [] + start.wait() + for item in it: + items.append(item) + results.extend(items) + + threads = self.run_threads(worker) + for t in threads: + t.join() + + self.assert_iterator_results(results, seq) + + def test_shared_iterator_exhaustion(self): + """Test concurrent exhaustion of a shared string iterator""" + for _ in range(100): + seq = self.make_testdata(NUMITEMS) + it = iter(seq) + start = threading.Barrier(NUMTHREADS) + + def worker(): + start.wait() + while True: + try: + next(it) + except StopIteration: + return + + threads = self.run_threads(worker) + for t in threads: + t.join() diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c index 2249280d0af811..76297676ea6cb9 100644 --- a/Objects/unicodeobject.c +++ b/Objects/unicodeobject.c @@ -55,6 +55,8 @@ OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. #include "pycore_pyhash.h" // _Py_HashSecret_t #include "pycore_pylifecycle.h" // _Py_SetFileSystemEncoding() #include "pycore_pystate.h" // _PyInterpreterState_GET() +#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_*() +#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION() #include "pycore_ucnhash.h" // _PyUnicode_Name_CAPI #include "pycore_unicodectype.h" // _PyUnicode_IsXidStart #include "pycore_unicodeobject.h" // struct _Py_unicode_state @@ -14968,21 +14970,30 @@ unicodeiter_next(PyObject *op) PyObject *seq; assert(it != NULL); +#ifdef Py_GIL_DISABLED + seq = _Py_atomic_load_ptr(&it->it_seq); +#else seq = it->it_seq; +#endif if (seq == NULL) return NULL; assert(_PyUnicode_CHECK(seq)); - if (it->it_index < PyUnicode_GET_LENGTH(seq)) { + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); + if (index < PyUnicode_GET_LENGTH(seq)) { int kind = PyUnicode_KIND(seq); const void *data = PyUnicode_DATA(seq); - Py_UCS4 chr = PyUnicode_READ(kind, data, it->it_index); - it->it_index++; + Py_UCS4 chr = PyUnicode_READ(kind, data, index); + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index + 1); return unicode_char(chr); } +#ifdef Py_GIL_DISABLED + seq = _Py_atomic_exchange_ptr(&it->it_seq, NULL); +#else it->it_seq = NULL; - Py_DECREF(seq); +#endif + Py_XDECREF(seq); return NULL; } @@ -14991,21 +15002,32 @@ unicode_ascii_iter_next(PyObject *op) { unicodeiterobject *it = (unicodeiterobject *)op; assert(it != NULL); +#ifdef Py_GIL_DISABLED + PyObject *seq = _Py_atomic_load_ptr(&it->it_seq); +#else PyObject *seq = it->it_seq; +#endif if (seq == NULL) { return NULL; } assert(_PyUnicode_CHECK(seq)); assert(PyUnicode_IS_COMPACT_ASCII(seq)); - if (it->it_index < PyUnicode_GET_LENGTH(seq)) { + + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); + if (index < PyUnicode_GET_LENGTH(seq)) { const void *data = ((void*)(_PyASCIIObject_CAST(seq) + 1)); - Py_UCS1 chr = (Py_UCS1)PyUnicode_READ(PyUnicode_1BYTE_KIND, - data, it->it_index); - it->it_index++; + Py_UCS1 chr = (Py_UCS1)PyUnicode_READ( + PyUnicode_1BYTE_KIND, data, index); + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index + 1); return (PyObject*)&_Py_SINGLETON(strings).ascii[chr]; } + +#ifdef Py_GIL_DISABLED + seq = _Py_atomic_exchange_ptr(&it->it_seq, NULL); +#else it->it_seq = NULL; - Py_DECREF(seq); +#endif + Py_XDECREF(seq); return NULL; } @@ -15014,8 +15036,22 @@ unicodeiter_len(PyObject *op, PyObject *Py_UNUSED(ignored)) { unicodeiterobject *it = (unicodeiterobject *)op; Py_ssize_t len = 0; +#ifdef Py_GIL_DISABLED + PyObject *seq; + Py_BEGIN_CRITICAL_SECTION(it); + seq = Py_XNewRef(it->it_seq); + Py_END_CRITICAL_SECTION(); + if (seq != NULL) { + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); + Py_ssize_t seq_len = PyUnicode_GET_LENGTH(seq); + if (index < seq_len) + len = seq_len - index; + Py_DECREF(seq); + } +#else if (it->it_seq) len = PyUnicode_GET_LENGTH(it->it_seq) - it->it_index; +#endif return PyLong_FromSsize_t(len); } @@ -15031,16 +15067,34 @@ unicodeiter_reduce(PyObject *op, PyObject *Py_UNUSED(ignored)) * call must be before access of iterator pointers. * see issue #101765 */ +#ifdef Py_GIL_DISABLED + PyObject *seq; + Py_ssize_t index; + Py_BEGIN_CRITICAL_SECTION(it); + seq = Py_XNewRef(it->it_seq); + index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); + Py_END_CRITICAL_SECTION(); + + if (seq != NULL) { + if (index < PyUnicode_GET_LENGTH(seq)) { + PyObject *result = Py_BuildValue("N(O)n", iter, seq, index); + Py_DECREF(seq); + return result; + } + Py_DECREF(seq); + } +#else if (it->it_seq != NULL) { return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index); - } else { - PyObject *u = _PyUnicode_GetEmpty(); - if (u == NULL) { - Py_XDECREF(iter); - return NULL; - } - return Py_BuildValue("N(N)", iter, u); } +#endif + + PyObject *u = _PyUnicode_GetEmpty(); + if (u == NULL) { + Py_XDECREF(iter); + return NULL; + } + return Py_BuildValue("N(N)", iter, u); } PyDoc_STRVAR(reduce_doc, "Return state information for pickling."); @@ -15057,7 +15111,7 @@ unicodeiter_setstate(PyObject *op, PyObject *state) index = 0; else if (index > PyUnicode_GET_LENGTH(it->it_seq)) index = PyUnicode_GET_LENGTH(it->it_seq); /* iterator truncated */ - it->it_index = index; + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index); } Py_RETURN_NONE; } From 9ba2b02915ee72b642261b85c584f54f712aee54 Mon Sep 17 00:00:00 2001 From: Bhuvansh Kataria Date: Sat, 15 Aug 2026 20:47:58 +0000 Subject: [PATCH 2/4] gh-153928: add NEWS entry --- .../2026-08-15-20-47-35.gh-issue-153928.FUi0YU.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-08-15-20-47-35.gh-issue-153928.FUi0YU.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-08-15-20-47-35.gh-issue-153928.FUi0YU.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-08-15-20-47-35.gh-issue-153928.FUi0YU.rst new file mode 100644 index 00000000000000..481288fec31d5c --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-08-15-20-47-35.gh-issue-153928.FUi0YU.rst @@ -0,0 +1 @@ +Fix data races in Unicode iterators in free-threaded builds. From 6f5362d3cfad027a31975b4fce3c619d142ce0c1 Mon Sep 17 00:00:00 2001 From: Bhuvansh Date: Sun, 16 Aug 2026 03:14:24 +0530 Subject: [PATCH 3/4] Remove unused critical section include --- Objects/unicodeobject.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c index 76297676ea6cb9..f374affd7a837a 100644 --- a/Objects/unicodeobject.c +++ b/Objects/unicodeobject.c @@ -56,7 +56,6 @@ OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. #include "pycore_pylifecycle.h" // _Py_SetFileSystemEncoding() #include "pycore_pystate.h" // _PyInterpreterState_GET() #include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_*() -#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION() #include "pycore_ucnhash.h" // _PyUnicode_Name_CAPI #include "pycore_unicodectype.h" // _PyUnicode_IsXidStart #include "pycore_unicodeobject.h" // struct _Py_unicode_state From 727c529fe94d3f7fb79d3a31669d070941293889 Mon Sep 17 00:00:00 2001 From: Bhuvansh Kataria Date: Fri, 23 Oct 2026 07:01:12 +0000 Subject: [PATCH 4/4] gh-153928: switch str iterator to defer release to dealloc --- Lib/test/test_iter.py | 28 +++++++++++++ Objects/unicodeobject.c | 90 +++++++++++++++++------------------------ 2 files changed, 65 insertions(+), 53 deletions(-) diff --git a/Lib/test/test_iter.py b/Lib/test/test_iter.py index 18e4b676c53236..645036f0fd4376 100644 --- a/Lib/test/test_iter.py +++ b/Lib/test/test_iter.py @@ -1137,6 +1137,34 @@ def test_iter_neg_setstate(self): self.assertEqual(next(it), 0) self.assertEqual(next(it), 1) + @cpython_only + def test_exhausted_str_iter_setstate_does_not_revive(self): + # gh-153928: a str iterator exhausted by normal iteration must stay + # exhausted, even if __setstate__() is called afterwards (as pickle + # does when restoring one). In free-threaded builds, the iterator's + # reference to the string is kept until the iterator itself is + # deallocated rather than dropped on exhaustion (to avoid racing + # with other threads also exhausting it -- see unicodeiter_next()), + # so __setstate__() can no longer rely on it_seq being NULL to + # detect "already permanently exhausted". See also gh-129139, the + # same bug for list/tuple iterators. + s = "hello" + it = iter(s) + self.assertEqual(list(it), list(s)) + with self.assertRaises(StopIteration): + next(it) + it.__setstate__(2) + with self.assertRaises(StopIteration): + next(it) + + # same invariant via a real pickle round-trip + it2 = iter(s) + for _ in s: + next(it2) + it3 = pickle.loads(pickle.dumps(it2)) + with self.assertRaises(StopIteration): + next(it3) + def test_free_after_iterating(self): check_free_after_iterating(self, iter, SequenceClass, (0,)) diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c index f374affd7a837a..81cad7ff6e16d5 100644 --- a/Objects/unicodeobject.c +++ b/Objects/unicodeobject.c @@ -14966,14 +14966,15 @@ static PyObject * unicodeiter_next(PyObject *op) { unicodeiterobject *it = (unicodeiterobject *)op; - PyObject *seq; assert(it != NULL); -#ifdef Py_GIL_DISABLED - seq = _Py_atomic_load_ptr(&it->it_seq); -#else - seq = it->it_seq; -#endif + /* it_seq is set once at construction and, in free-threaded builds, + * released only in unicodeiter_dealloc() -- never here -- so reading + * it needs no atomic/critical-section protection: a thread calling + * next() always holds its own reference to `it`, which is exactly + * what keeps dealloc() from running concurrently. This mirrors + * listiter_next()/tupleiter_next(). */ + PyObject *seq = it->it_seq; if (seq == NULL) return NULL; assert(_PyUnicode_CHECK(seq)); @@ -14987,12 +14988,10 @@ unicodeiter_next(PyObject *op) return unicode_char(chr); } -#ifdef Py_GIL_DISABLED - seq = _Py_atomic_exchange_ptr(&it->it_seq, NULL); -#else +#ifndef Py_GIL_DISABLED it->it_seq = NULL; + Py_DECREF(seq); #endif - Py_XDECREF(seq); return NULL; } @@ -15001,11 +15000,7 @@ unicode_ascii_iter_next(PyObject *op) { unicodeiterobject *it = (unicodeiterobject *)op; assert(it != NULL); -#ifdef Py_GIL_DISABLED - PyObject *seq = _Py_atomic_load_ptr(&it->it_seq); -#else - PyObject *seq = it->it_seq; -#endif + PyObject *seq = it->it_seq; /* see unicodeiter_next() for why this is safe unsynchronized */ if (seq == NULL) { return NULL; } @@ -15021,12 +15016,10 @@ unicode_ascii_iter_next(PyObject *op) return (PyObject*)&_Py_SINGLETON(strings).ascii[chr]; } -#ifdef Py_GIL_DISABLED - seq = _Py_atomic_exchange_ptr(&it->it_seq, NULL); -#else +#ifndef Py_GIL_DISABLED it->it_seq = NULL; + Py_DECREF(seq); #endif - Py_XDECREF(seq); return NULL; } @@ -15035,22 +15028,14 @@ unicodeiter_len(PyObject *op, PyObject *Py_UNUSED(ignored)) { unicodeiterobject *it = (unicodeiterobject *)op; Py_ssize_t len = 0; -#ifdef Py_GIL_DISABLED - PyObject *seq; - Py_BEGIN_CRITICAL_SECTION(it); - seq = Py_XNewRef(it->it_seq); - Py_END_CRITICAL_SECTION(); + PyObject *seq = it->it_seq; if (seq != NULL) { Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); Py_ssize_t seq_len = PyUnicode_GET_LENGTH(seq); - if (index < seq_len) + if (index < seq_len) { len = seq_len - index; - Py_DECREF(seq); + } } -#else - if (it->it_seq) - len = PyUnicode_GET_LENGTH(it->it_seq) - it->it_index; -#endif return PyLong_FromSsize_t(len); } @@ -15066,27 +15051,13 @@ unicodeiter_reduce(PyObject *op, PyObject *Py_UNUSED(ignored)) * call must be before access of iterator pointers. * see issue #101765 */ -#ifdef Py_GIL_DISABLED - PyObject *seq; - Py_ssize_t index; - Py_BEGIN_CRITICAL_SECTION(it); - seq = Py_XNewRef(it->it_seq); - index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); - Py_END_CRITICAL_SECTION(); - + PyObject *seq = it->it_seq; if (seq != NULL) { + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); if (index < PyUnicode_GET_LENGTH(seq)) { - PyObject *result = Py_BuildValue("N(O)n", iter, seq, index); - Py_DECREF(seq); - return result; + return Py_BuildValue("N(O)n", iter, seq, index); } - Py_DECREF(seq); - } -#else - if (it->it_seq != NULL) { - return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index); } -#endif PyObject *u = _PyUnicode_GetEmpty(); if (u == NULL) { @@ -15105,12 +15076,25 @@ unicodeiter_setstate(PyObject *op, PyObject *state) Py_ssize_t index = PyLong_AsSsize_t(state); if (index == -1 && PyErr_Occurred()) return NULL; - if (it->it_seq != NULL) { - if (index < 0) - index = 0; - else if (index > PyUnicode_GET_LENGTH(it->it_seq)) - index = PyUnicode_GET_LENGTH(it->it_seq); /* iterator truncated */ - FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index); + PyObject *seq = it->it_seq; + if (seq != NULL) { + /* A permanently exhausted iterator must stay exhausted: in + * free-threaded builds it_seq is never cleared on exhaustion + * (see unicodeiter_next()), so unlike the GIL build we can't + * use "it_seq == NULL" to detect that. Use the index instead: + * once it has reached the sequence length, refuse to move it + * backwards. Without this check, __setstate__() could "revive" + * an exhausted iterator -- the same bug reported against list + * and tuple iterators in gh-129139. */ + Py_ssize_t cur_index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); + Py_ssize_t len = PyUnicode_GET_LENGTH(seq); + if (cur_index < len) { + if (index < 0) + index = 0; + else if (index > len) + index = len; /* iterator truncated */ + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index); + } } Py_RETURN_NONE; }