From 61839ca123693cb7b172fa2aed55f6343bf02605 Mon Sep 17 00:00:00 2001 From: Ahmed Magdy Ali Date: Thu, 28 May 2026 11:25:03 +0300 Subject: [PATCH 1/4] gh-150505: fix data race in Unpickler_set_memo under free-threading Unpickler_set_memo modified self->memo and self->memo_size without any critical section, allowing concurrent threads to race on the same Unpickler instance. Wrap the memo population loop and the pointer swap in Py_BEGIN_CRITICAL_SECTION/Py_END_CRITICAL_SECTION to serialize access. --- Lib/test/test_pickle.py | 15 +++++++ ...-05-28-11-46-23.gh-issue-150505.avtcRy.rst | 3 ++ Modules/_pickle.c | 41 ++++++++++++++++--- 3 files changed, 53 insertions(+), 6 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2026-05-28-11-46-23.gh-issue-150505.avtcRy.rst diff --git a/Lib/test/test_pickle.py b/Lib/test/test_pickle.py index 48375cf459ea0b1..2d567c20ce44df3 100644 --- a/Lib/test/test_pickle.py +++ b/Lib/test/test_pickle.py @@ -17,6 +17,7 @@ from test import support from test.support import cpython_only, import_helper, os_helper from test.support.import_helper import ensure_lazy_imports +from test.support import threading_helper from test.pickletester import AbstractHookTests from test.pickletester import AbstractUnpickleTests @@ -808,6 +809,20 @@ def test_unknown_flag(self): self.assertStartsWith(stderr.getvalue(), 'usage: ') +class UnpicklerMemoThreadSafetyTest(unittest.TestCase): + @unittest.skipUnless(support.Py_GIL_DISABLED, 'this test can only possibly fail with GIL disabled') + @threading_helper.reap_threads + @threading_helper.requires_working_threading() + def test_memo_assignment_race(self): + from threading import Thread + shared = pickle.Unpickler(io.BytesIO(b'')) + def assign(): + for _ in range(10000): + shared.memo = {j: j for j in range(100)} + threads = [Thread(target=assign) for _ in range(8)] + for t in threads: t.start() + for t in threads: t.join() + def load_tests(loader, tests, pattern): tests.addTest(doctest.DocTestSuite(pickle)) return tests diff --git a/Misc/NEWS.d/next/Library/2026-05-28-11-46-23.gh-issue-150505.avtcRy.rst b/Misc/NEWS.d/next/Library/2026-05-28-11-46-23.gh-issue-150505.avtcRy.rst new file mode 100644 index 000000000000000..216bf85f6f0d815 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-05-28-11-46-23.gh-issue-150505.avtcRy.rst @@ -0,0 +1,3 @@ +Fix data race in :class:`pickle.Unpickler` when multiple threads assign to +the ``memo`` attribute concurrently under free-threading. The memo swap and +population loop are now protected with a critical section. diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 2d82438f0d308d9..846b356a7a4a0f4 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -7786,24 +7786,38 @@ Unpickler_set_memo(PyObject *op, PyObject *obj, void *Py_UNUSED(closure)) if (new_memo == NULL) return -1; + bool goto_error = false; + Py_BEGIN_CRITICAL_SECTION(self); while (PyDict_Next(obj, &i, &key, &value)) { Py_ssize_t idx; if (!PyLong_Check(key)) { PyErr_SetString(PyExc_TypeError, "memo key must be integers"); - goto error; + goto_error = true; + break; } idx = PyLong_AsSsize_t(key); - if (idx == -1 && PyErr_Occurred()) - goto error; + if (idx == -1 && PyErr_Occurred()) { + goto_error = true; + break; + } if (idx < 0) { PyErr_SetString(PyExc_ValueError, "memo key must be positive integers."); - goto error; + goto_error = true; + break; } + if (_Unpickler_MemoPut(self, idx, value) < 0) - goto error; + { + goto_error = true; + break; + } } + Py_END_CRITICAL_SECTION(); + if (goto_error) + goto error; + } else { PyErr_Format(PyExc_TypeError, @@ -7812,9 +7826,24 @@ Unpickler_set_memo(PyObject *op, PyObject *obj, void *Py_UNUSED(closure)) return -1; } - _Unpickler_MemoCleanup(self); + Py_ssize_t old_memo_size, i; + PyObject **old_memo; + + Py_BEGIN_CRITICAL_SECTION(self); + old_memo_size = self->memo_size; + old_memo = self->memo; self->memo_size = new_memo_size; self->memo = new_memo; + Py_END_CRITICAL_SECTION(); + + if (old_memo == NULL) { + return 0; + } + i = old_memo_size; + while (--i >= 0) { + Py_XDECREF(old_memo[i]); + } + PyMem_Free(old_memo); return 0; From 6f465f9da92306937a82c0544b1541f642fa80dd Mon Sep 17 00:00:00 2001 From: Ahmed Magdy Ali Date: Fri, 29 May 2026 00:28:10 +0300 Subject: [PATCH 2/4] gh-150505: Revert "fix data race in Unpickler_set_memo under free-threading" --- Lib/test/test_pickle.py | 15 ------- ...-05-28-11-46-23.gh-issue-150505.avtcRy.rst | 3 -- Modules/_pickle.c | 41 +++---------------- 3 files changed, 6 insertions(+), 53 deletions(-) delete mode 100644 Misc/NEWS.d/next/Library/2026-05-28-11-46-23.gh-issue-150505.avtcRy.rst diff --git a/Lib/test/test_pickle.py b/Lib/test/test_pickle.py index 2d567c20ce44df3..48375cf459ea0b1 100644 --- a/Lib/test/test_pickle.py +++ b/Lib/test/test_pickle.py @@ -17,7 +17,6 @@ from test import support from test.support import cpython_only, import_helper, os_helper from test.support.import_helper import ensure_lazy_imports -from test.support import threading_helper from test.pickletester import AbstractHookTests from test.pickletester import AbstractUnpickleTests @@ -809,20 +808,6 @@ def test_unknown_flag(self): self.assertStartsWith(stderr.getvalue(), 'usage: ') -class UnpicklerMemoThreadSafetyTest(unittest.TestCase): - @unittest.skipUnless(support.Py_GIL_DISABLED, 'this test can only possibly fail with GIL disabled') - @threading_helper.reap_threads - @threading_helper.requires_working_threading() - def test_memo_assignment_race(self): - from threading import Thread - shared = pickle.Unpickler(io.BytesIO(b'')) - def assign(): - for _ in range(10000): - shared.memo = {j: j for j in range(100)} - threads = [Thread(target=assign) for _ in range(8)] - for t in threads: t.start() - for t in threads: t.join() - def load_tests(loader, tests, pattern): tests.addTest(doctest.DocTestSuite(pickle)) return tests diff --git a/Misc/NEWS.d/next/Library/2026-05-28-11-46-23.gh-issue-150505.avtcRy.rst b/Misc/NEWS.d/next/Library/2026-05-28-11-46-23.gh-issue-150505.avtcRy.rst deleted file mode 100644 index 216bf85f6f0d815..000000000000000 --- a/Misc/NEWS.d/next/Library/2026-05-28-11-46-23.gh-issue-150505.avtcRy.rst +++ /dev/null @@ -1,3 +0,0 @@ -Fix data race in :class:`pickle.Unpickler` when multiple threads assign to -the ``memo`` attribute concurrently under free-threading. The memo swap and -population loop are now protected with a critical section. diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 846b356a7a4a0f4..2d82438f0d308d9 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -7786,38 +7786,24 @@ Unpickler_set_memo(PyObject *op, PyObject *obj, void *Py_UNUSED(closure)) if (new_memo == NULL) return -1; - bool goto_error = false; - Py_BEGIN_CRITICAL_SECTION(self); while (PyDict_Next(obj, &i, &key, &value)) { Py_ssize_t idx; if (!PyLong_Check(key)) { PyErr_SetString(PyExc_TypeError, "memo key must be integers"); - goto_error = true; - break; + goto error; } idx = PyLong_AsSsize_t(key); - if (idx == -1 && PyErr_Occurred()) { - goto_error = true; - break; - } + if (idx == -1 && PyErr_Occurred()) + goto error; if (idx < 0) { PyErr_SetString(PyExc_ValueError, "memo key must be positive integers."); - goto_error = true; - break; + goto error; } - if (_Unpickler_MemoPut(self, idx, value) < 0) - { - goto_error = true; - break; - } + goto error; } - Py_END_CRITICAL_SECTION(); - if (goto_error) - goto error; - } else { PyErr_Format(PyExc_TypeError, @@ -7826,24 +7812,9 @@ Unpickler_set_memo(PyObject *op, PyObject *obj, void *Py_UNUSED(closure)) return -1; } - Py_ssize_t old_memo_size, i; - PyObject **old_memo; - - Py_BEGIN_CRITICAL_SECTION(self); - old_memo_size = self->memo_size; - old_memo = self->memo; + _Unpickler_MemoCleanup(self); self->memo_size = new_memo_size; self->memo = new_memo; - Py_END_CRITICAL_SECTION(); - - if (old_memo == NULL) { - return 0; - } - i = old_memo_size; - while (--i >= 0) { - Py_XDECREF(old_memo[i]); - } - PyMem_Free(old_memo); return 0; From aef265f4eaf356d0603a755dfc615a136ebfad35 Mon Sep 17 00:00:00 2001 From: Ahmed Magdy Ali Date: Fri, 29 May 2026 01:05:46 +0300 Subject: [PATCH 3/4] gh-150505: document that Pickler and Unpickler are not thread-safe Sharing Pickler or Unpickler instances across threads has no defined semantics. The internal state (memo, stream position, stack) forms a single coherent parse/serialize state that cannot be meaningfully used across threads simultaneously. --- Doc/library/pickle.rst | 12 ++++++++++++ .../2026-05-29-01-05-14.gh-issue-150505.qOSg_U.rst | 3 +++ 2 files changed, 15 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2026-05-29-01-05-14.gh-issue-150505.qOSg_U.rst diff --git a/Doc/library/pickle.rst b/Doc/library/pickle.rst index 8eadc2cf2b1ef0d..c673b0506bcaf78 100644 --- a/Doc/library/pickle.rst +++ b/Doc/library/pickle.rst @@ -316,6 +316,12 @@ The :mod:`!pickle` module exports three classes, :class:`Pickler`, It is an error if *buffer_callback* is not ``None`` and *protocol* is ``None`` or smaller than 5. + .. note:: + :class:`Pickler` objects are not thread-safe. Sharing a single + :class:`Pickler` instance across multiple threads without external + synchronization leads to undefined behavior. Use a separate instance + per thread. + .. versionchanged:: 3.8 The *buffer_callback* argument was added. @@ -430,6 +436,12 @@ The :mod:`!pickle` module exports three classes, :class:`Pickler`, an :ref:`out-of-band ` buffer view. Such buffers have been given in order to the *buffer_callback* of a Pickler object. + .. note:: + :class:`Unpickler` objects are not thread-safe. Sharing a single + :class:`Unpickler` instance across multiple threads without external + synchronization leads to undefined behavior. Use a separate instance + per thread. + .. versionchanged:: 3.8 The *buffers* argument was added. diff --git a/Misc/NEWS.d/next/Library/2026-05-29-01-05-14.gh-issue-150505.qOSg_U.rst b/Misc/NEWS.d/next/Library/2026-05-29-01-05-14.gh-issue-150505.qOSg_U.rst new file mode 100644 index 000000000000000..5468993e56baa27 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-05-29-01-05-14.gh-issue-150505.qOSg_U.rst @@ -0,0 +1,3 @@ +Document that :class:`pickle.Pickler` and :class:`pickle.Unpickler` +instances are not thread-safe. Sharing instances across threads without +external synchronization leads to undefined behavior. From a540d413dd5ed4e8dfbf1ed258c996ed206b3ea5 Mon Sep 17 00:00:00 2001 From: Ahmed Magdy Ali Date: Fri, 29 May 2026 01:18:43 +0300 Subject: [PATCH 4/4] gh-150505: remove unnecessary NEWS entry for docs-only change --- .../Library/2026-05-29-01-05-14.gh-issue-150505.qOSg_U.rst | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 Misc/NEWS.d/next/Library/2026-05-29-01-05-14.gh-issue-150505.qOSg_U.rst diff --git a/Misc/NEWS.d/next/Library/2026-05-29-01-05-14.gh-issue-150505.qOSg_U.rst b/Misc/NEWS.d/next/Library/2026-05-29-01-05-14.gh-issue-150505.qOSg_U.rst deleted file mode 100644 index 5468993e56baa27..000000000000000 --- a/Misc/NEWS.d/next/Library/2026-05-29-01-05-14.gh-issue-150505.qOSg_U.rst +++ /dev/null @@ -1,3 +0,0 @@ -Document that :class:`pickle.Pickler` and :class:`pickle.Unpickler` -instances are not thread-safe. Sharing instances across threads without -external synchronization leads to undefined behavior.