From b652577405947720bff2097b7ac83978ca0b0681 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 10 Feb 2025 20:09:35 +0000 Subject: [PATCH 1/5] gh-129967: Fix race condition in `repr(set)` The call to `PySequence_List()` could temporarily unlock and relock the set, allowing the items to be cleared and return the incorrect notation `{}` for a empty set (it should be `set()`). --- Lib/test/test_free_threading/test_set.py | 43 +++++++++++++++++++ ...-02-10-20-01-56.gh-issue-129967.J60tEl.rst | 2 + Objects/setobject.c | 11 ++++- 3 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 Lib/test/test_free_threading/test_set.py create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-02-10-20-01-56.gh-issue-129967.J60tEl.rst diff --git a/Lib/test/test_free_threading/test_set.py b/Lib/test/test_free_threading/test_set.py new file mode 100644 index 00000000000000..b5f3a1b33b9c73 --- /dev/null +++ b/Lib/test/test_free_threading/test_set.py @@ -0,0 +1,43 @@ +import unittest + +from threading import Thread, Barrier +from unittest import TestCase + +from test.support import threading_helper + + +@threading_helper.requires_working_threading() +class TestSet(TestCase): + def test_repr_clear(self): + """Test repr() of a set while another thread is calling clear()""" + NUM_ITERS = 10 + NUM_REPR_THREADS = 10 + barrier = Barrier(NUM_REPR_THREADS + 1) + s = {1, 2, 3, 4, 5, 6, 7, 8} + set_reprs = [] + + def clear_set(): + barrier.wait() + s.clear() + + def repr_set(): + nonlocal set_repr + barrier.wait() + set_reprs.append(repr(s)) + + for _ in range(NUM_ITERS): + set_reprs = [] + threads = [Thread(target=clear_set)] + for _ in range(NUM_REPR_THREADS): + threads.append(Thread(target=repr_set)) + for t in threads: + t.start() + for t in threads: + t.join() + + for set_repr in set_reprs: + self.assertIn(set_repr, ("set()", "{1, 2, 3, 4, 5, 6, 7, 8}")) + + +if __name__ == "__main__": + unittest.main() diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-02-10-20-01-56.gh-issue-129967.J60tEl.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-02-10-20-01-56.gh-issue-129967.J60tEl.rst new file mode 100644 index 00000000000000..3bd8707ba994ad --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-02-10-20-01-56.gh-issue-129967.J60tEl.rst @@ -0,0 +1,2 @@ +Fix a race condition in the :term:`free threaded` build when ``repr(set)`` +is called concurrently with ``set.clear()``. diff --git a/Objects/setobject.c b/Objects/setobject.c index 26ab352ca6d989..b45687bf4c8513 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -535,9 +535,16 @@ set_repr_lock_held(PySetObject *so) return PyUnicode_FromFormat("%s()", Py_TYPE(so)->tp_name); } - keys = PySequence_List((PyObject *)so); - if (keys == NULL) + keys = PyList_New(so->used); + if (keys == NULL) { goto done; + } + + Py_ssize_t pos = 0, idx = 0; + setentry *entry; + while (set_next(so, &pos, &entry)) { + PyList_SET_ITEM(keys, idx++, Py_NewRef(entry->key)); + } /* repr(keys)[1:-1] */ listrepr = PyObject_Repr(keys); From 2f04291d32341585f0b494fbe555abaa088ca4d5 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 10 Feb 2025 20:17:52 +0000 Subject: [PATCH 2/5] Update blurb --- .../2025-02-10-20-01-56.gh-issue-129967.J60tEl.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-02-10-20-01-56.gh-issue-129967.J60tEl.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-02-10-20-01-56.gh-issue-129967.J60tEl.rst index 3bd8707ba994ad..69ec03d2ae3824 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2025-02-10-20-01-56.gh-issue-129967.J60tEl.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-02-10-20-01-56.gh-issue-129967.J60tEl.rst @@ -1,2 +1,2 @@ -Fix a race condition in the :term:`free threaded` build when ``repr(set)`` +Fix a race condition in the :term:`free threading` build when ``repr(set)`` is called concurrently with ``set.clear()``. From d12d8b60cc16431fab8146428d5f1c1cc1527ad3 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 11 Feb 2025 14:43:54 -0500 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: T. Wouters --- Lib/test/test_free_threading/test_set.py | 9 ++++----- Objects/setobject.c | 2 ++ 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_free_threading/test_set.py b/Lib/test/test_free_threading/test_set.py index b5f3a1b33b9c73..eb0c2275106c02 100644 --- a/Lib/test/test_free_threading/test_set.py +++ b/Lib/test/test_free_threading/test_set.py @@ -14,7 +14,7 @@ def test_repr_clear(self): NUM_REPR_THREADS = 10 barrier = Barrier(NUM_REPR_THREADS + 1) s = {1, 2, 3, 4, 5, 6, 7, 8} - set_reprs = [] + expected = {repr(set()), repr(s)} def clear_set(): barrier.wait() @@ -23,10 +23,10 @@ def clear_set(): def repr_set(): nonlocal set_repr barrier.wait() - set_reprs.append(repr(s)) + set_reprs.add(repr(s)) for _ in range(NUM_ITERS): - set_reprs = [] + set_reprs = set() threads = [Thread(target=clear_set)] for _ in range(NUM_REPR_THREADS): threads.append(Thread(target=repr_set)) @@ -35,8 +35,7 @@ def repr_set(): for t in threads: t.join() - for set_repr in set_reprs: - self.assertIn(set_repr, ("set()", "{1, 2, 3, 4, 5, 6, 7, 8}")) + self.assertEqual(set_reprs, expected) if __name__ == "__main__": diff --git a/Objects/setobject.c b/Objects/setobject.c index b45687bf4c8513..902b6fbe1d91e3 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -535,6 +535,8 @@ set_repr_lock_held(PySetObject *so) return PyUnicode_FromFormat("%s()", Py_TYPE(so)->tp_name); } + // Avoid PySequence_List because it might re-lock the object lock or + // the GIL and allow something to clear the set from underneath us. keys = PyList_New(so->used); if (keys == NULL) { goto done; From 1efd4e56375749f6c65fe7f9fc6dc1b0d0c2be82 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 11 Feb 2025 19:47:59 +0000 Subject: [PATCH 4/5] Revert changes to test_set.py --- Lib/test/test_free_threading/test_set.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_free_threading/test_set.py b/Lib/test/test_free_threading/test_set.py index eb0c2275106c02..a66e03bcc4c9d1 100644 --- a/Lib/test/test_free_threading/test_set.py +++ b/Lib/test/test_free_threading/test_set.py @@ -14,19 +14,17 @@ def test_repr_clear(self): NUM_REPR_THREADS = 10 barrier = Barrier(NUM_REPR_THREADS + 1) s = {1, 2, 3, 4, 5, 6, 7, 8} - expected = {repr(set()), repr(s)} def clear_set(): barrier.wait() s.clear() def repr_set(): - nonlocal set_repr barrier.wait() - set_reprs.add(repr(s)) + set_reprs.append(repr(s)) for _ in range(NUM_ITERS): - set_reprs = set() + set_reprs = [] threads = [Thread(target=clear_set)] for _ in range(NUM_REPR_THREADS): threads.append(Thread(target=repr_set)) @@ -35,7 +33,8 @@ def repr_set(): for t in threads: t.join() - self.assertEqual(set_reprs, expected) + for set_repr in set_reprs: + self.assertIn(set_repr, ("set()", "{1, 2, 3, 4, 5, 6, 7, 8}")) if __name__ == "__main__": From ddb87fe6fc0c8b7918ca4bb006d3545777330159 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 11 Feb 2025 19:54:13 +0000 Subject: [PATCH 5/5] Add reference to GH issue --- Objects/setobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/setobject.c b/Objects/setobject.c index 902b6fbe1d91e3..1978ae2b165d18 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -535,8 +535,8 @@ set_repr_lock_held(PySetObject *so) return PyUnicode_FromFormat("%s()", Py_TYPE(so)->tp_name); } - // Avoid PySequence_List because it might re-lock the object lock or - // the GIL and allow something to clear the set from underneath us. + // gh-129967: avoid PySequence_List because it might re-lock the object + // lock or the GIL and allow something to clear the set from underneath us. keys = PyList_New(so->used); if (keys == NULL) { goto done;