Skip to content

Commit 61ba7ff

Browse files
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.
1 parent f4b1d3e commit 61ba7ff

2 files changed

Lines changed: 10 additions & 1 deletion

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Fix undefined behaviour in ``_PyObject_MergePerThreadRefcounts()`` on the
2+
free-threaded build. The per-thread reference count delta being merged is
3+
routinely negative, and it was shifted left by ``_Py_REF_SHARED_SHIFT``,
4+
which is undefined behaviour for a negative value. The shift is now done in
5+
the unsigned domain.

Python/uniqueid.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,8 +181,12 @@ _PyObject_MergePerThreadRefcounts(_PyThreadStateImpl *tstate)
181181
Py_ssize_t refcnt = tstate->refcounts.values[i];
182182
if (refcnt != 0) {
183183
PyObject *obj = pool->table[i].obj;
184+
/* `refcnt` is a per-thread delta, so it is routinely negative,
185+
and shifting a negative value left is undefined behaviour.
186+
Shift in the unsigned domain instead. */
184187
_Py_atomic_add_ssize(&obj->ob_ref_shared,
185-
refcnt << _Py_REF_SHARED_SHIFT);
188+
(Py_ssize_t)((size_t)refcnt
189+
<< _Py_REF_SHARED_SHIFT));
186190
tstate->refcounts.values[i] = 0;
187191
}
188192
}

0 commit comments

Comments
 (0)