From 932eb3f4b67ac9c56d76799a54d20befc13b5208 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 18 Jun 2026 11:29:34 +0200 Subject: [PATCH 1/9] 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 5ca6362406a78b9..523dec00d8b4a30 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" @@ -2595,24 +2595,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 fd6512004de151d1d3b5e3008436f1ea38ffc090 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 18 Jun 2026 11:33:08 +0200 Subject: [PATCH 2/9] 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 000000000000000..7c1c7cdda8772f7 --- /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 6f175edcb1abdb8562acee22fb010263fcb9bc26 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 18 Jun 2026 11:33:30 +0200 Subject: [PATCH 3/9] 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 7c1c7cdda8772f7..89fdf33965179ef 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 4c72daeacd08fc1398b40ddbe7c96ec84324661d Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 18 Jun 2026 11:34:22 +0200 Subject: [PATCH 4/9] 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 849b0480e232fc2..f9788654940aac3 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 2e15be54b602a5c00c069c57ddd3c4887bb7983a Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 18 Jun 2026 14:11:56 +0200 Subject: [PATCH 5/9] 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 523dec00d8b4a30..8e4525570ea1365 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -2612,7 +2612,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 0b0f4940c8a13bd818fcfa2596cc660a868059fe Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 9 Jul 2026 16:05:05 +0200 Subject: [PATCH 6/9] 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 f9788654940aac3..1985d842eadc7fa 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 ec593dfed114f2458c8e56fc1c2f8038a1caf480 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 9 Jul 2026 16:05:24 +0200 Subject: [PATCH 7/9] 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 1985d842eadc7fa..d86b058c36e7898 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 9d01cc7d691c4231c7f7c9440bb0def695f85255 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 9 Jul 2026 16:12:53 +0200 Subject: [PATCH 8/9] 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 d86b058c36e7898..0f3432dabe9ebc0 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 192fc9f8ca6e3d2888f48bc7503cfe2015390ac0 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Thu, 9 Jul 2026 16:14:50 +0200 Subject: [PATCH 9/9] 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 8e4525570ea1365..5dac3a9804db8b6 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -2548,7 +2548,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; @@ -2597,6 +2596,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); @@ -2641,6 +2641,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 { @@ -2649,9 +2650,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); } } @@ -2661,7 +2663,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;