Repository navigation
_Py_SetImmortal must be run on allocating thread (no-gil) #113956
Description
Bug report
Bug description:
The code in _Py_SetImmortal must be run on the thread that originally allocated the object. If this is not the case, then a race between Py_INCREF can cause the _Py_SetImmortal to be ignored.
cpython/Include/internal/pycore_object.h
Lines 128 to 139 in 2e7577b
| static inline void _Py_SetImmortal(PyObject *op) | |
| { | |
| if (op) { | |
| #ifdef Py_GIL_DISABLED | |
| op->ob_tid = _Py_UNOWNED_TID; | |
| op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL; | |
| op->ob_ref_shared = 0; | |
| #else | |
| op->ob_refcnt = _Py_IMMORTAL_REFCNT; | |
| #endif | |
| } | |
| } |
Example sequence:
- Allocating thread begins an
_Py_Incref. - Allocating thread detects it should perform inc ref on
ob_ref_local, and calculates new value. - Non-allocating thread runs all of
_Py_SetImmortal. - Allocating thread completes incref with assignment to
ob_ref_local, and unsets the value from immortal.
In the nogil fork, there is some defensive code on other operations that need to be run on the allocating thread, e.g.
_PyObject_SetDeferredRefcount(PyObject *op)
{
assert(_Py_ThreadLocal(op) && "non thread-safe");I think something like this should occur inside _Py_SetImmortal.
It looks like _Py_SetImmortal is not exposed outside the runtime. However, PEP 683, says:
Then setting any object’s refcount to _Py_IMMORTAL_REFCNT makes it immortal.
This implies there is an intent to expose it, and with the changes to nogil. It seems that this should be exposed as an explicit operation, rather than by setting the ref count directly.
CC @ericsnowcurrently @colesbury
P.S. I am happy to PR any required changes.
CPython versions tested on:
CPython main branch
Operating systems tested on:
No response
Activity
colesbury commentedon Jan 11, 2024
A few thoughts:
Py_INCREF()concurrently with another thread trying to immortalize the object, that will result in a race condition_Py_SetImmortal()is not public and I don't think we should make it public (at least not in 3.13)colesbury commentedon Jan 11, 2024
I would be hesitant to add an assertion to
_Py_SetImmortal()because we call it from a number of places where we already know we are in a bad state, likenone_dealloc.The only call I've seen that concerns me is in
_PyUnicode_InternInPlace(). In the free-threaded builds, we should probably only immortalize the string if it is owned by the current thread.mjp41 commentedon Jan 12, 2024
@colesbury thanks for the reply. This makes sense. I had thought this is more externally reachable.
I think you could make it concurrency safe by borrowing a third bit from
ob_ref_sharedto represent Immortal.colesbury commentedon Jan 12, 2024
Yeah, that would address concurrency safety, but using
ob_ref_localserves two other purposes:ob_ref_local32-bits without having to worry about overflow, because the immortal value isUINT32_MAX.Py_INCREFandPy_DECREFfaster because we only need to accessob_tidandob_ref_local.devdanzin commentedon Apr 10, 2026
Adding a concrete reproducer for this race. The scenario @mjp41 described and @colesbury confirmed above is reachable from pure-Python code via concurrent
setattr()on shared objects with fresh string keys — exactly the_PyUnicode_InternInPlacecaller Sam flagged as the concerning one.Reproducer (pure stdlib, no extensions)
Observed results
Reproduced on two independent free-threaded debug builds, in every run tried (4 runs total, 2 per build):
heads/3.14-dirty:ea9ecc8955a, Apr 6 2026Objects/unicodeobject.c:778heads/main:4d0e8ee649c, Mar 29 2026Objects/unicodeobject.c:691(same check; file reorganized)Both builds: Clang 21.1.2, Linux x86_64,
--disable-gil --with-pydebug. The 3.14 build additionally has--with-thread-sanitizer; the 3.15 build has-fsanitize=address.The SIGABRT in both cases is the debug consistency check at
_PyUnicode_CheckConsistency:The object's
ob_ref_localhas been set to_Py_IMMORTAL_REFCNT_LOCAL(3221225472=0xC0000000) by one thread, while another thread is observing it in theSSTATE_INTERNED_MORTALstate — the invariant the consistency check enforces.TSan on the 3.14 build flags the underlying race directly. Writer (Thread T8, inside
intern_commonafterLOCK_INTERNED):Reader (Thread T1, still in
PyObject_SetAttrbefore enteringintern_common):Same address (
ob_ref_localof the interned string), same step sequence @mjp41 described in the original post: one thread is mid-_Py_SetImmortal, another thread'sPy_INCREFis concurrently reading the field.Notes
SSTATE_INTERNED_MORTAL→!immortalinvariant._Py_SetImmortal(s)to execute before the atomic publication ofSSTATE_INTERNED_IMMORTAL. The comment added in that PR explicitly documents the ordering as the synchronization point forintern_common's lock-free fast path — which makes the race window visible in the source. I don't think gh-146041: Avoid lock in sys.intern() for already interned strings #146072 introduced the race (thePy_INCREFatPyObject_SetAttr:1449has always been concurrent withimmortalize_internedunder the lock), but the new layout makes it easier to reason about._PyUnicode_CheckConsistency(Objects/unicodeobject.c:760-763on 3.14,:675-676on 3.15) calls the "object observed in INTERNED_MORTAL state turns out to be immortal" case "extremely unlikely (worth a potential failed assertion in a debug build)." The reproducer above suggests "extremely unlikely" is too strong — 8 threads and a fresh key per iteration is enough to hit it on first try.No fix proposed — I'm not familiar enough with the FT internals to judge between making the writes atomic, relaxing the debug assertion, or requiring the owning-thread discipline Matthew originally suggested. Happy to provide full TSan output or test any candidate fix.
Found while investigating TSan results from a C extension (
wrapt) using labeille and ft-review-toolkit; isolated to pure stdlib after ruling out the extension's own races.Root cause analysis, reproducer creation and comment draft done with assistance from Claude Code, reviewed by a human.
ngoldbaum commentedon Apr 21, 2026
@colesbury it seems a test in the CFFI thread sanitizer CI is able to reliably trigger a race related to this, see python-cffi/cffi#242. I'm not able to reproduce this race on 3.15. It also seems to have started happening relatively recently. Maybe something was included in a 3.14 backport that makes triggering this easier?
Since it's fixed in 3.15 and appears to be harmless for CFFI, my plan right now for CFFI is to just add a suppression for this.
colesbury commentedon Apr 21, 2026
It's not fully fixed in 3.15 either
gh-113956: Make intern_common thread-safe in free-threaded build (gh-…
[3.14] gh-113956: Make intern_common thread-safe in free-threaded bui…
colesbury commentedon Apr 23, 2026
I'm closing the issue now that:
sys.intern, etc. are now thread-safe in the way that they use_Py_SetImmortalPyUnstable_SetImmortalis limited to_PyObject_IsUniquelyReferenced()pythongh-113956: Make intern_common thread-safe in free-threaded build (