From 1230545a923bafb9a89195920800115a52f232ef Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sun, 25 May 2025 23:36:59 +1200 Subject: [PATCH 1/5] gh-116909: fix data race with versions in typeobject Global state `_PyRuntime.types.next_version_tag` is being accessed without synchronization or atomics. This could potentially result in the same version being used in two different type objects, incrementing past the maximum limit, and the usual non-atomic memory access issues. Fix this by using atomics to ensure the version is accessed and updated in a race-free manner, while also ensuring it is never incremented past the expected (maximum + 1). Note there is a theoretical change in behaviour with the second use, for static builtin types, if their versions exceed the maximum, with assertions disabled: previously they would have continued incrementing past the maximum and using the increasing version number; now they will all use the maximum. It might be better to replace the `assert` with an `abort()`: I assume we would be crashing shortly if we continue after this, both before and after this change. --- Lib/test/test_capi/test_misc.py | 1 - ...-05-25-23-55-43.gh-issue-116909.FGbNKx.rst | 2 ++ Objects/typeobject.c | 32 ++++++++++++++++--- 3 files changed, 29 insertions(+), 6 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-05-25-23-55-43.gh-issue-116909.FGbNKx.rst diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index f74694a7a745a4..cc06711264ba2d 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1413,7 +1413,6 @@ def run_tasks(): self.assertNotIn(task.requester_tid, runner_tids) @requires_subinterpreters - @support.skip_if_sanitizer("gh-129824: race on assign_version_tag", thread=True) def test_isolated_subinterpreter(self): # We exercise the most important permutations. diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-25-23-55-43.gh-issue-116909.FGbNKx.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-25-23-55-43.gh-issue-116909.FGbNKx.rst new file mode 100644 index 00000000000000..57457bc573cba8 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-25-23-55-43.gh-issue-116909.FGbNKx.rst @@ -0,0 +1,2 @@ +Access and update ``_PyRuntime.types.next_version_tag`` in a thread-safe +manner. diff --git a/Objects/typeobject.c b/Objects/typeobject.c index a7ab69fef4c721..19d47c1bc40fb4 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -54,7 +54,6 @@ class object "PyObject *" "&PyBaseObject_Type" PyUnicode_CheckExact(name) && \ (PyUnicode_GET_LENGTH(name) <= MCACHE_MAX_ATTR_SIZE) -#define NEXT_GLOBAL_VERSION_TAG _PyRuntime.types.next_version_tag #define NEXT_VERSION_TAG(interp) \ (interp)->types.next_version_tag @@ -1258,6 +1257,25 @@ _PyType_GetVersionForCurrentState(PyTypeObject *tp) #error "_Py_ATTR_CACHE_UNUSED must be bigger than max" #endif +static int +get_next_global_version(unsigned int *dest) +{ + unsigned int v; + while (1) { + v = _Py_atomic_load_uint_relaxed(&_PyRuntime.types.next_version_tag); + + /* Stop if passed the maximum or we successfully updated the field */ + if (v > _Py_MAX_GLOBAL_TYPE_VERSION_TAG || + _Py_atomic_compare_exchange_uint(&_PyRuntime.types.next_version_tag, + &v, v + 1)) { + break; + } + } + + *dest = v; + return v <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG; +} + static int assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) { @@ -1288,11 +1306,12 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) } if (type->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) { /* static types */ - if (NEXT_GLOBAL_VERSION_TAG > _Py_MAX_GLOBAL_TYPE_VERSION_TAG) { + unsigned int version; + if (!get_next_global_version(&version)) { /* We have run out of version numbers */ return 0; } - set_version_unlocked(type, NEXT_GLOBAL_VERSION_TAG++); + set_version_unlocked(type, version); assert (type->tp_version_tag <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG); } else { @@ -8877,9 +8896,12 @@ init_static_type(PyInterpreterState *interp, PyTypeObject *self, type_add_flags(self, _Py_TPFLAGS_STATIC_BUILTIN); type_add_flags(self, Py_TPFLAGS_IMMUTABLETYPE); - assert(NEXT_GLOBAL_VERSION_TAG <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG); + unsigned int version; + if (!get_next_global_version(&version)) { + assert("we have run out of version numbers"); + } if (self->tp_version_tag == 0) { - _PyType_SetVersion(self, NEXT_GLOBAL_VERSION_TAG++); + _PyType_SetVersion(self, version); } } else { From b3a63c878579fe2c35041f7526bae9bbe32eb5af Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Mon, 26 May 2025 16:45:19 +1200 Subject: [PATCH 2/5] Continue to skip test_isolated_subinterpreter under TSAN This change may fix one issue, but others remain --- Lib/test/test_capi/test_misc.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index cc06711264ba2d..f74694a7a745a4 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1413,6 +1413,7 @@ def run_tasks(): self.assertNotIn(task.requester_tid, runner_tids) @requires_subinterpreters + @support.skip_if_sanitizer("gh-129824: race on assign_version_tag", thread=True) def test_isolated_subinterpreter(self): # We exercise the most important permutations. From 33adfc379cda8c3e7fdadc1ad65f7bfafc4c91d0 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Mon, 26 May 2025 11:44:32 +1200 Subject: [PATCH 3/5] Correct assertion that should always fail if reached --- Objects/typeobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 19d47c1bc40fb4..04758b8ad56d3e 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -8898,7 +8898,7 @@ init_static_type(PyInterpreterState *interp, PyTypeObject *self, unsigned int version; if (!get_next_global_version(&version)) { - assert("we have run out of version numbers"); + assert(0 && "we have run out of version numbers"); } if (self->tp_version_tag == 0) { _PyType_SetVersion(self, version); From d9e1c23084b5b75de9f2430a9f0672b84989e7d3 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Mon, 26 May 2025 11:45:40 +1200 Subject: [PATCH 4/5] Don't allocate a global type version tag unless required --- Objects/typeobject.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 04758b8ad56d3e..ae76f56f988272 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -8896,11 +8896,11 @@ init_static_type(PyInterpreterState *interp, PyTypeObject *self, type_add_flags(self, _Py_TPFLAGS_STATIC_BUILTIN); type_add_flags(self, Py_TPFLAGS_IMMUTABLETYPE); - unsigned int version; - if (!get_next_global_version(&version)) { - assert(0 && "we have run out of version numbers"); - } if (self->tp_version_tag == 0) { + unsigned int version; + if (!get_next_global_version(&version)) { + assert(0 && "we have run out of version numbers"); + } _PyType_SetVersion(self, version); } } From 1eb121d105ca3c1a4aa601a3051e91bbdb4a0a8c Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Tue, 27 May 2025 10:05:35 +1200 Subject: [PATCH 5/5] Update Misc/NEWS.d/next/Core_and_Builtins/2025-05-25-23-55-43.gh-issue-116909.FGbNKx.rst Co-authored-by: Peter Bierma --- .../2025-05-25-23-55-43.gh-issue-116909.FGbNKx.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-25-23-55-43.gh-issue-116909.FGbNKx.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-25-23-55-43.gh-issue-116909.FGbNKx.rst index 57457bc573cba8..c11fbc88a45d5d 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-25-23-55-43.gh-issue-116909.FGbNKx.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-25-23-55-43.gh-issue-116909.FGbNKx.rst @@ -1,2 +1 @@ -Access and update ``_PyRuntime.types.next_version_tag`` in a thread-safe -manner. +Fix crash upon importing a static type in a subinterpreter.