From ed9df6627abe0e6e66003582124e7dba11112aa1 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 18 Jun 2026 11:29:34 +0200 Subject: [PATCH 01/13] fix: avoid use after free in Counter.update --- Modules/_collectionsmodule.c | 46 +++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/Modules/_collectionsmodule.c b/Modules/_collectionsmodule.c index 62d1dad5735ec8..3feeb6d2e3aede 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -1,6 +1,6 @@ #include "Python.h" #include "pycore_call.h" // _PyObject_CallNoArgs() -#include "pycore_dict.h" // _PyDict_GetItem_KnownHash() +#include "pycore_dict.h" // _PyDict_GetItemRef_KnownHash_LockHeld() #include "pycore_long.h" // _PyLong_GetZero() #include "pycore_moduleobject.h" // _PyModule_GetState() #include "pycore_pyatomic_ft_wrappers.h" @@ -2594,24 +2594,36 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, goto done; } - oldval = _PyDict_GetItem_KnownHash(mapping, key, hash); - if (oldval == NULL) { - if (PyErr_Occurred()) - goto done; - if (_PyDict_SetItem_KnownHash(mapping, key, one, hash) < 0) - goto done; - } else { - /* oldval is a borrowed reference. Keep it alive across - PyNumber_Add(), which can execute arbitrary user code and - mutate (or even clear) the underlying dict. */ - Py_INCREF(oldval); + int found; + int cs_err = 0; + Py_BEGIN_CRITICAL_SECTION(mapping); + found = _PyDict_GetItemRef_KnownHash_LockHeld( + (PyDictObject *)mapping, key, hash, &oldval); + if (found < 0) { + cs_err = -1; + } + else if (found == 0) { + if (_PyDict_SetItem_KnownHash_LockHeld( + (PyDictObject *)mapping, key, one, hash) < 0) { + cs_err = -1; + } + } + else { newval = PyNumber_Add(oldval, one); Py_DECREF(oldval); - if (newval == NULL) - goto done; - if (_PyDict_SetItem_KnownHash(mapping, key, newval, hash) < 0) - goto done; - Py_CLEAR(newval); + oldval = NULL; + if (newval == NULL) { + cs_err = -1; + } + else if (_PyDict_SetItem_KnownHash_LockHeld( + (PyDictObject *)mapping, key, newval, hash) < 0) { + cs_err = -1; + } + } + Py_END_CRITICAL_SECTION(); + Py_CLEAR(newval); + if (cs_err < 0) { + goto done; } Py_DECREF(key); } From f4896c45ce2a5719c89a85b38c33cff237e4e476 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 18 Jun 2026 11:33:08 +0200 Subject: [PATCH 02/13] misc: add news entry --- .../next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst diff --git a/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst b/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst new file mode 100644 index 00000000000000..7c1c7cdda8772f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst @@ -0,0 +1 @@ +Fix a crash under free-threading when multiple threads update the same :class:`~collections.Counter` concurrently. From 1d32413c59490ee221881a099b67878d51e80fd4 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 18 Jun 2026 11:33:30 +0200 Subject: [PATCH 03/13] misc: add news entry --- .../next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst b/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst index 7c1c7cdda8772f..89fdf33965179e 100644 --- a/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst +++ b/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst @@ -1 +1 @@ -Fix a crash under free-threading when multiple threads update the same :class:`~collections.Counter` concurrently. +Fix a race condition under free-threading when multiple threads update the same :class:`~collections.Counter` concurrently. From ad068b2216522952ab46ffa8b714cf133e35659a Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 18 Jun 2026 11:34:22 +0200 Subject: [PATCH 04/13] test: add regression test --- .../test_free_threading/test_collections.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_free_threading/test_collections.py b/Lib/test/test_free_threading/test_collections.py index 849b0480e232fc..f9788654940aac 100644 --- a/Lib/test/test_free_threading/test_collections.py +++ b/Lib/test/test_free_threading/test_collections.py @@ -1,5 +1,6 @@ +import threading import unittest -from collections import deque +from collections import Counter, deque from copy import copy from test.support import threading_helper @@ -49,5 +50,21 @@ def mutate(): ) +class TestCounter(unittest.TestCase): + def test_update_concurrent(self): + # gh-151633: concurrent Counter.update calls must not cause use-after-free + # under free-threading. + NTHREADS = 4 + PER_THREAD = 5000 + c = Counter() + data = ['x'] * PER_THREAD + threads = [threading.Thread(target=c.update, args=(data,)) + for _ in range(NTHREADS)] + for t in threads: + t.start() + for t in threads: + t.join() + + if __name__ == "__main__": unittest.main() From 423506e290dc4d1fb6485ec3af31ce3f19a7a689 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 18 Jun 2026 14:11:56 +0200 Subject: [PATCH 05/13] review: update Modules/_collectionsmodule.c Co-authored-by: Pieter Eendebak --- Modules/_collectionsmodule.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Modules/_collectionsmodule.c b/Modules/_collectionsmodule.c index 3feeb6d2e3aede..a365893693c27b 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -2611,7 +2611,6 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, else { newval = PyNumber_Add(oldval, one); Py_DECREF(oldval); - oldval = NULL; if (newval == NULL) { cs_err = -1; } From 0cdf16a07fe626b82a790f4b13c81cff8047b239 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 9 Jul 2026 16:05:05 +0200 Subject: [PATCH 06/13] review: update Lib/test/test_free_threading/test_collections.py Co-authored-by: Pieter Eendebak --- Lib/test/test_free_threading/test_collections.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_free_threading/test_collections.py b/Lib/test/test_free_threading/test_collections.py index f9788654940aac..1985d842eadc7f 100644 --- a/Lib/test/test_free_threading/test_collections.py +++ b/Lib/test/test_free_threading/test_collections.py @@ -53,7 +53,6 @@ def mutate(): class TestCounter(unittest.TestCase): def test_update_concurrent(self): # gh-151633: concurrent Counter.update calls must not cause use-after-free - # under free-threading. NTHREADS = 4 PER_THREAD = 5000 c = Counter() From 4eb2a35be3bb37824d49550d1245976a9b5759ce Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 9 Jul 2026 16:05:24 +0200 Subject: [PATCH 07/13] review: update Lib/test/test_free_threading/test_collections.py Co-authored-by: Pieter Eendebak --- Lib/test/test_free_threading/test_collections.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_free_threading/test_collections.py b/Lib/test/test_free_threading/test_collections.py index 1985d842eadc7f..d86b058c36e789 100644 --- a/Lib/test/test_free_threading/test_collections.py +++ b/Lib/test/test_free_threading/test_collections.py @@ -56,7 +56,7 @@ def test_update_concurrent(self): NTHREADS = 4 PER_THREAD = 5000 c = Counter() - data = ['x'] * PER_THREAD + data = ('x',) * PER_THREAD threads = [threading.Thread(target=c.update, args=(data,)) for _ in range(NTHREADS)] for t in threads: From 679c606e03fe59594a7ab3fd2a1b158d00b88505 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 9 Jul 2026 16:12:53 +0200 Subject: [PATCH 08/13] review: use run_concurrently --- Lib/test/test_free_threading/test_collections.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_free_threading/test_collections.py b/Lib/test/test_free_threading/test_collections.py index d86b058c36e789..0f3432dabe9ebc 100644 --- a/Lib/test/test_free_threading/test_collections.py +++ b/Lib/test/test_free_threading/test_collections.py @@ -1,4 +1,3 @@ -import threading import unittest from collections import Counter, deque from copy import copy @@ -57,12 +56,9 @@ def test_update_concurrent(self): PER_THREAD = 5000 c = Counter() data = ('x',) * PER_THREAD - threads = [threading.Thread(target=c.update, args=(data,)) - for _ in range(NTHREADS)] - for t in threads: - t.start() - for t in threads: - t.join() + threading_helper.run_concurrently( + c.update, nthreads=NTHREADS, args=(data,) + ) if __name__ == "__main__": From c34c4a25c87164356e5b3d5a76ff1f1523d32618 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 9 Jul 2026 16:14:50 +0200 Subject: [PATCH 09/13] review: update Modules/_collectionsmodule.c --- Modules/_collectionsmodule.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Modules/_collectionsmodule.c b/Modules/_collectionsmodule.c index a365893693c27b..23a69a1c1121f0 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -2547,7 +2547,6 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, /*[clinic end generated code: output=7e0c1789636b3d8f input=e79fad04534a0b45]*/ { PyObject *it, *oldval; - PyObject *newval = NULL; PyObject *key = NULL; PyObject *bound_get = NULL; PyObject *mapping_get; @@ -2596,6 +2595,7 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, int found; int cs_err = 0; + PyObject *newval = NULL; Py_BEGIN_CRITICAL_SECTION(mapping); found = _PyDict_GetItemRef_KnownHash_LockHeld( (PyDictObject *)mapping, key, hash, &oldval); @@ -2640,6 +2640,7 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, oldval = PyObject_CallFunctionObjArgs(bound_get, key, zero, NULL); if (oldval == NULL) break; + PyObject *newval; if (oldval == zero) { newval = Py_NewRef(one); } else { @@ -2648,9 +2649,10 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, Py_DECREF(oldval); if (newval == NULL) break; - if (PyObject_SetItem(mapping, key, newval) < 0) + int status = PyObject_SetItem(mapping, key, newval); + Py_DECREF(newval); + if (status < 0) break; - Py_CLEAR(newval); Py_DECREF(key); } } @@ -2660,7 +2662,6 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, Py_XDECREF(mapping_setitem); Py_DECREF(it); Py_XDECREF(key); - Py_XDECREF(newval); Py_XDECREF(bound_get); if (PyErr_Occurred()) return NULL; From d2a050f3f8afefde9e4984b87c216f9838579143 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Sat, 24 Oct 2026 18:50:33 +0200 Subject: [PATCH 10/13] refactor: move logic to separate function --- Modules/_collectionsmodule.c | 122 +++++++++++++++++++---------------- 1 file changed, 68 insertions(+), 54 deletions(-) diff --git a/Modules/_collectionsmodule.c b/Modules/_collectionsmodule.c index 23a69a1c1121f0..28092b2a73539a 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -1,5 +1,6 @@ #include "Python.h" #include "pycore_call.h" // _PyObject_CallNoArgs() +#include "pycore_critical_section.h" // _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED() #include "pycore_dict.h" // _PyDict_GetItemRef_KnownHash_LockHeld() #include "pycore_long.h" // _PyLong_GetZero() #include "pycore_moduleobject.h" // _PyModule_GetState() @@ -2531,6 +2532,67 @@ static PyType_Spec defdict_spec = { /* helper function for Counter *********************************************/ +/* Fast path advantages: + 1. Eliminate double hashing + (by re-using the same hash for both the get and set) + 2. Avoid argument overhead of PyObject_CallFunctionObjArgs + (argument tuple creation and parsing) + 3. Avoid indirection through a bound method object + (creates another argument tuple) + 4. Avoid initial increment from zero + (reuse an existing one-object instead) +*/ +static int +_count_elements_dict_lock_held(PyDictObject *mapping, PyObject *it) +{ + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(mapping); + PyObject *one = _PyLong_GetOne(); // borrowed reference + + while (1) { + Py_hash_t hash; + PyObject *oldval; + PyObject *newval; + int status; + + PyObject *key = PyIter_Next(it); + if (key == NULL) { + return PyErr_Occurred() ? -1 : 0; + } + + hash = _PyObject_HashDictKey(key); + if (hash == -1) { + Py_DECREF(key); + return -1; + } + + int found = _PyDict_GetItemRef_KnownHash_LockHeld(mapping, key, hash, + &oldval); + if (found < 0) { + Py_DECREF(key); + return -1; + } + if (found == 0) { + status = _PyDict_SetItem_KnownHash_LockHeld(mapping, key, one, + hash); + } + else { + newval = PyNumber_Add(oldval, one); + Py_DECREF(oldval); + if (newval == NULL) { + Py_DECREF(key); + return -1; + } + status = _PyDict_SetItem_KnownHash_LockHeld(mapping, key, newval, + hash); + Py_DECREF(newval); + } + Py_DECREF(key); + if (status < 0) { + return -1; + } + } +} + /*[clinic input] _collections._count_elements @@ -2571,60 +2633,12 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping, mapping_setitem != NULL && mapping_setitem == dict_setitem && PyDict_Check(mapping)) { - while (1) { - /* Fast path advantages: - 1. Eliminate double hashing - (by re-using the same hash for both the get and set) - 2. Avoid argument overhead of PyObject_CallFunctionObjArgs - (argument tuple creation and parsing) - 3. Avoid indirection through a bound method object - (creates another argument tuple) - 4. Avoid initial increment from zero - (reuse an existing one-object instead) - */ - Py_hash_t hash; - - key = PyIter_Next(it); - if (key == NULL) - break; - - hash = _PyObject_HashDictKey(key); - if (hash == -1) { - goto done; - } - - int found; - int cs_err = 0; - PyObject *newval = NULL; - Py_BEGIN_CRITICAL_SECTION(mapping); - found = _PyDict_GetItemRef_KnownHash_LockHeld( - (PyDictObject *)mapping, key, hash, &oldval); - if (found < 0) { - cs_err = -1; - } - else if (found == 0) { - if (_PyDict_SetItem_KnownHash_LockHeld( - (PyDictObject *)mapping, key, one, hash) < 0) { - cs_err = -1; - } - } - else { - newval = PyNumber_Add(oldval, one); - Py_DECREF(oldval); - if (newval == NULL) { - cs_err = -1; - } - else if (_PyDict_SetItem_KnownHash_LockHeld( - (PyDictObject *)mapping, key, newval, hash) < 0) { - cs_err = -1; - } - } - Py_END_CRITICAL_SECTION(); - Py_CLEAR(newval); - if (cs_err < 0) { - goto done; - } - Py_DECREF(key); + int status; + Py_BEGIN_CRITICAL_SECTION(mapping); + status = _count_elements_dict_lock_held((PyDictObject *)mapping, it); + Py_END_CRITICAL_SECTION(); + if (status < 0) { + goto done; } } else { From 3727427ea9389caf93e2143dc95983ee8eea7b0e Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Sat, 24 Oct 2026 18:57:02 +0200 Subject: [PATCH 11/13] test: assert actual result --- Lib/test/test_free_threading/test_collections.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_free_threading/test_collections.py b/Lib/test/test_free_threading/test_collections.py index 4af799fe3763f3..d6efde55e698d0 100644 --- a/Lib/test/test_free_threading/test_collections.py +++ b/Lib/test/test_free_threading/test_collections.py @@ -59,6 +59,7 @@ def test_update_concurrent(self): threading_helper.run_concurrently( c.update, nthreads=NTHREADS, args=(data,) ) + self.assertEqual(c['x'], NTHREADS * PER_THREAD) class TestOrderedDict(unittest.TestCase): From c68e85eb144bbbda172a7a239b1d4505d3b89acc Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Sat, 24 Oct 2026 18:57:22 +0200 Subject: [PATCH 12/13] chore: update news entry --- .../Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst b/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst index 89fdf33965179e..73d533c0d109bb 100644 --- a/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst +++ b/Misc/NEWS.d/next/Library/2026-06-18-11-26-00.gh-issue-151633.uJzHdc.rst @@ -1 +1,2 @@ -Fix a race condition under free-threading when multiple threads update the same :class:`~collections.Counter` concurrently. +Fix a use-after-free under free-threading when multiple threads +update the same :class:`~collections.Counter` concurrently. From 303b81b3ac96cd9b2c5ae69d78a2804e3205e48f Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Tue, 27 Oct 2026 21:35:49 +0100 Subject: [PATCH 13/13] Update Modules/_collectionsmodule.c --- Modules/_collectionsmodule.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/_collectionsmodule.c b/Modules/_collectionsmodule.c index 28092b2a73539a..5a8a49c48424da 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -2551,7 +2551,6 @@ _count_elements_dict_lock_held(PyDictObject *mapping, PyObject *it) while (1) { Py_hash_t hash; PyObject *oldval; - PyObject *newval; int status; PyObject *key = PyIter_Next(it); @@ -2576,7 +2575,7 @@ _count_elements_dict_lock_held(PyDictObject *mapping, PyObject *it) hash); } else { - newval = PyNumber_Add(oldval, one); + PyObject *newval = PyNumber_Add(oldval, one); Py_DECREF(oldval); if (newval == NULL) { Py_DECREF(key);