From a4cda41759a09973c79301fcb6cdc62bb59f2e1f Mon Sep 17 00:00:00 2001 From: Matthias Goergens Date: Thu, 30 Jul 2026 14:25:46 +0800 Subject: [PATCH] gh-148286: Fix undefined behaviour in per-thread refcount merging (free-threaded) _PyObject_MergePerThreadRefcounts() merges each thread's local reference count deltas into the shared count: _Py_atomic_add_ssize(&obj->ob_ref_shared, refcnt << _Py_REF_SHARED_SHIFT); `refcnt` is a delta, so it is routinely negative -- the observed value is almost always -1 -- and shifting a negative value left is undefined behaviour. Do the shift in the unsigned domain and convert back. This is not reachable from the UBSan CI job today: the sanitizer matrix in .github/workflows/build.yml pairs UBSan only with free-threading: false, so this file is never built under UBSan. Building --disable-gil with --with-undefined-behavior-sanitizer shows how load-bearing it is; the very first line of output from `./python -c pass` is the UBSan report, raised from interpreter startup via _PyImport_InitExternal, and it recurs from gc_collect_main on every collection. Measured over the full test suite on that configuration, with all entries in Tools/ubsan/suppressions.txt disabled: before: 1762 UB reports, 529 test failures across 61 test files after: 0 UB reports, 0 failures, suite green (run=51,517) Most of those failures are tests that assert a subprocess produced no stderr, which the UBSan diagnostic breaks. Adding free-threading: true to the UBSan matrix would keep this fixed; that is left as a separate decision since it costs a CI job. --- .../2026-07-30-14-30-00.gh-issue-148286.Wn5Tz4.rst | 5 +++++ Python/uniqueid.c | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-07-30-14-30-00.gh-issue-148286.Wn5Tz4.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-07-30-14-30-00.gh-issue-148286.Wn5Tz4.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-07-30-14-30-00.gh-issue-148286.Wn5Tz4.rst new file mode 100644 index 000000000000000..5c467ffd8577d01 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-07-30-14-30-00.gh-issue-148286.Wn5Tz4.rst @@ -0,0 +1,5 @@ +Fix undefined behaviour in ``_PyObject_MergePerThreadRefcounts()`` on the +free-threaded build. The per-thread reference count delta being merged is +routinely negative, and it was shifted left by ``_Py_REF_SHARED_SHIFT``, +which is undefined behaviour for a negative value. The shift is now done in +the unsigned domain. diff --git a/Python/uniqueid.c b/Python/uniqueid.c index 64c3e6cfbbe8250..e6e2c3eadcb434e 100644 --- a/Python/uniqueid.c +++ b/Python/uniqueid.c @@ -181,8 +181,12 @@ _PyObject_MergePerThreadRefcounts(_PyThreadStateImpl *tstate) Py_ssize_t refcnt = tstate->refcounts.values[i]; if (refcnt != 0) { PyObject *obj = pool->table[i].obj; + /* `refcnt` is a per-thread delta, so it is routinely negative, + and shifting a negative value left is undefined behaviour. + Shift in the unsigned domain instead. */ _Py_atomic_add_ssize(&obj->ob_ref_shared, - refcnt << _Py_REF_SHARED_SHIFT); + (Py_ssize_t)((size_t)refcnt + << _Py_REF_SHARED_SHIFT)); tstate->refcounts.values[i] = 0; } }