Repository navigation
GC in free-threaded build has problems with Py_INCREF()/Py_DECREF() in tp_traverse handlers #123241
Description
Bug report
This came up in the context of nanobind. Nanobind implements (in a test) a traverse function:
int funcwrapper_tp_traverse(PyObject *self, visitproc visit, void *arg) {
FuncWrapper *w = nb::inst_ptr<FuncWrapper>(self);
nb::object f = nb::cast(w->f, nb::rv_policy::none);
Py_VISIT(f.ptr());
return 0;
};The nb::object smart pointer is internally reference counted. In other words, the above is roughly equivalent to:
int funcwrapper_tp_traverse(PyObject *self, visitproc visit, void *arg) {
PyObject *f = self->w->f;
Py_INCREF(f);
Py_VISIT(f);
Py_DECREF(f);
return 0;
};This leads to a leak in the free-threaded GC for subtle reasons: when determining resurrected objects, the free-threaded GC uses ob_ref_local to compute the refcount - incoming references, which may be (temporarily) negative. In this case, Py_INCREF() adds 1 to the refcount, but by the time Py_DECREF() is called, the local refcount is -1 which makes the object appear immortal.
There are a number of limitations on the implementations of traverse functions, which are not well documented. For example, it's not safe to allocate, free, track, or untrack Python objects. It's unclear to me whether there are other issues with calling refcounting functions in traverse callbacks.
I think we can make handle_resurrected_objects more robust to this by splitting the first pass over state->unreachable into two passes.
See also: PyO3/pyo3#3165, which was not related to the free-threaded build.
Linked PRs
- gh-123241: Don't modify ref count during visitation #142232
- gh-123241: Make gc_free_threading.c more robust to increfs/decrefs in tp_traverse #142271
- gh-123241: Document restrictions for tp_traverse implementations #142272
- [3.13] gh-123241: Document restrictions for tp_traverse implementations (gh-142272) #142422
- [3.14] gh-123241: Document restrictions for tp_traverse implementations (gh-142272) #142423
- [3.14] gh-123241: Don't modify ref count during visitation (GH-142232) #142567
Activity
pablogsal commented on Aug 22, 2024
There are two things here to address:
I can try to give a go to both of these. For (2) we can potentially "lock" the GC into a state that emits a warning of some sort if the GC heads are modified during traversals or if any GC function that we don't expect it's called. WDYT?
pablogsal commented on Aug 22, 2024
Can you elaborate on this to double check that we are in the same page?
colesbury commented on Aug 22, 2024
The pass over the unreachable (but possibly resurrected) objects computes the incoming references in ob_ref_local. The final value is always >= 0, but it can be temporarily negative. (Well, sort of "negative". It's a uint32_t so it wraps around.) This is related to the problem seen by nanobind because -1 (or UINT32_MAX) looks like "immortal".
cpython/Python/gc_free_threading.c
Lines 1004 to 1013 in 297f2e0
We can change this so that the first pass initializes ob_ref_local to the current refcount (less one because the GC in this case holds a strong reference). The second pass than calls the traverse handlers. In this structure, the traverse handlers never see a ob_ref_local that looks like an immortal object.
I am still not convinced that it's safe (at least in the free-threaded build) to call Py_INCREF/DECREF during GC traversal, but this at least would fix the leak in nanobind. (I'd characterize it as more robust, rather than "safe".)
... // Subtract one to account for the reference from the worklist. op->ob_ref_local += (uint32_t)refcount - 1; } WORKSTACK_FOR_EACH(&state->unreachable, op) { traverseproc traverse = Py_TYPE(op)->tp_traverse; (void) traverse(op, (visitproc)visit_decref_unreachable, NULL); }pablogsal commented on Aug 22, 2024
I think for the general issue I would like to:
What are your thoughts?
colesbury commented on Aug 22, 2024
To be clear; the problem in this instance was the use of Py_INCREF/Py_DECREF rather than things like PyObject_GC_Track, although those would definitely be problematic in a tp_traverse.
It seems fine to add extra checks in debug builds, but I'd be worried about introduce slow downs in optimized builds for many of these functions. We probably would also need to be careful about how we report warnings and errors. I think creating a Python warning or error object during a tp_traverse handler execution may cause the same sort of problems we're trying to avoid.
I'm also hesitant to rush any fixes. The change to nanobind seems rather straightforward. And @ngoldbaum has run into something that might be related in PyO3 -- I don't fully understand that problem yet.
pablogsal commented on Aug 22, 2024
I share the same concerns so don't worry. In any case all of these proposal would be only on debug builds and is just a way to warn about the problem. I don't think we need to "fix" anything other than the docs to specify the contract. Any other thing is going to add slowdowns or extra complexity in release builds
davidhewitt commented on Feb 6, 2025
For what it's worth the PyO3 issue was indeed a refcounting op hidden behind a smart pointer (credit @colesbury for identifying that at the time). PyO3/pyo3#4479
pythongh-123241: Make gc_free_threading.c more robust to increfs/decr…
pythongh-123241: Make gc_free_threading.c more robust to increfs/decr…
pythongh-123241: Document restrictions for tp_traverse implementations
gh-123241: Document restrictions for `tp_traverse` implementations (g…
pythongh-123241: Document restrictions for `tp_traverse` implementati…
pythongh-123241: Document restrictions for `tp_traverse` implementati…
Merge branch 'main' into pythongh-123241-traverse
[3.14] gh-123241: Document restrictions for `tp_traverse` implementat…
[3.13] gh-123241: Document restrictions for `tp_traverse` implementat…
gh-123241: Don't modify ref count during visitation (GH-142232)
[3.14] pythongh-123241: Don't modify ref count during visitation (pyt…
[3.14] gh-123241: Don't modify ref count during visitation (GH-142232) (
encukou commented on Dec 16, 2025
This means that tp_traverse must not fail (because it can't set an exception), right?
Does tp_clear have similar requirements?
colesbury commented on Dec 16, 2025
I think "the traversal function must not have any side effects" means that tp_traverse cannot itself fail or set an exception.
However, the visitor passed to tp_traverse can fail and that failure is passed up via Py_VISIT. For example, the gc.get_referrers visitor appends to a list, which can fail with an out of memory error.
In other words, the GC is in control of whether the tp_traverse call can fail and, in some places, the GC assumes the call must not fail. (This is true in both the FT and default build GCs -- look for (void) traverse)
tp_clear can return -1 and it can set an exception. However, the return code has always been ignored (even in Python 2.0). Any exception is printed as unraisable and cleared.
I think it generally makes more sense for the tp_clear implementation to handle it's own internal exceptions (like tp_dealloc implementations are required to), but it doesn't really matter.
encukou commented on Dec 17, 2025
A topic for adding API that would be safe to use in tp_traverse: https://discuss.python.org/t/105331
davidhewitt commented on Dec 19, 2025
Related: #142981