https://github.com/python/cpython/commit/f963239ff1f986742d4c6bab2ab7b73f5a4047f6
commit: f963239ff1f986742d4c6bab2ab7b73f5a4047f6
branch: main
author: Sam Gross <colesb...@gmail.com>
committer: colesbury <colesb...@gmail.com>
date: 2025-02-25T12:03:28-05:00
summary:
gh-130202: Fix bug in `_PyObject_ResurrectEnd` in free threaded build
(gh-130281)
This fixes a fairly subtle bug involving finalizers and resurrection in
debug free threaded builds: if `_PyObject_ResurrectEnd` returns `1`
(i.e., the object was resurrected by a finalizer), it's not safe to
access the object because it might still be deallocated. For example:
* The finalizer may have exposed the object to another thread. That
thread may hold the last reference and concurrently deallocate it any
time after `_PyObject_ResurrectEnd()` returns `1`.
* `_PyObject_ResurrectEnd()` may call `_Py_brc_queue_object()`, which
may internally deallocate the object immediately if the owning thread
is dead.
Therefore, it's important not to access the object after it's
resurrected. We only violate this in two cases, and only in debug
builds:
* We assert that the object is tracked appropriately. This is now moved
up betewen the finalizer and the `_PyObject_ResurrectEnd()` call.
* The `--with-trace-refs` builds may need to remember the object if
it's resurrected. This is now handled by `_PyObject_ResurrectStart()`
and `_PyObject_ResurrectEnd()`.
Note that `--with-trace-refs` is currently disabled in `--disable-gil`
builds because the refchain hash table isn't thread-safe, but this
refactoring avoids an additional thread-safety issue.
files:
M Include/cpython/object.h
M Include/internal/pycore_object.h
M Objects/object.c
diff --git a/Include/cpython/object.h b/Include/cpython/object.h
index 260b90da24c18b..f466091e07e465 100644
--- a/Include/cpython/object.h
+++ b/Include/cpython/object.h
@@ -5,6 +5,7 @@
PyAPI_FUNC(void) _Py_NewReference(PyObject *op);
PyAPI_FUNC(void) _Py_NewReferenceNoTotal(PyObject *op);
PyAPI_FUNC(void) _Py_ResurrectReference(PyObject *op);
+PyAPI_FUNC(void) _Py_ForgetReference(PyObject *op);
#ifdef Py_REF_DEBUG
/* These are useful as debugging aids when chasing down refleaks. */
diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h
index ffd31bd4a27f49..53403ebcfc0043 100644
--- a/Include/internal/pycore_object.h
+++ b/Include/internal/pycore_object.h
@@ -730,6 +730,9 @@ _PyObject_ResurrectStart(PyObject *op)
#else
Py_SET_REFCNT(op, 1);
#endif
+#ifdef Py_TRACE_REFS
+ _Py_ResurrectReference(op);
+#endif
}
// Undoes an object resurrection by decrementing the refcount without calling
@@ -743,13 +746,22 @@ _PyObject_ResurrectEnd(PyObject *op)
#endif
#ifndef Py_GIL_DISABLED
Py_SET_REFCNT(op, Py_REFCNT(op) - 1);
- return Py_REFCNT(op) != 0;
+ if (Py_REFCNT(op) == 0) {
+# ifdef Py_TRACE_REFS
+ _Py_ForgetReference(op);
+# endif
+ return 0;
+ }
+ return 1;
#else
uint32_t local = _Py_atomic_load_uint32_relaxed(&op->ob_ref_local);
Py_ssize_t shared = _Py_atomic_load_ssize_acquire(&op->ob_ref_shared);
if (_Py_IsOwnedByCurrentThread(op) && local == 1 && shared == 0) {
// Fast-path: object has a single refcount and is owned by this thread
_Py_atomic_store_uint32_relaxed(&op->ob_ref_local, 0);
+# ifdef Py_TRACE_REFS
+ _Py_ForgetReference(op);
+# endif
return 0;
}
// Slow-path: object has a shared refcount or is not owned by this thread
diff --git a/Objects/object.c b/Objects/object.c
index d342549b6ffecc..b3309bac7afdee 100644
--- a/Objects/object.c
+++ b/Objects/object.c
@@ -496,10 +496,22 @@ _PyObject_ResurrectEndSlow(PyObject *op)
// merge the refcount. This isn't necessary in all cases, but it
// simplifies the implementation.
Py_ssize_t refcount = _Py_ExplicitMergeRefcount(op, -1);
- return refcount != 0;
+ if (refcount == 0) {
+#ifdef Py_TRACE_REFS
+ _Py_ForgetReference(op);
+#endif
+ return 0;
+ }
+ return 1;
}
int is_dead = _Py_DecRefSharedIsDead(op, NULL, 0);
- return !is_dead;
+ if (is_dead) {
+#ifdef Py_TRACE_REFS
+ _Py_ForgetReference(op);
+#endif
+ return 0;
+ }
+ return 1;
}
@@ -589,20 +601,24 @@ PyObject_CallFinalizerFromDealloc(PyObject *self)
Py_REFCNT(self) > 0,
"refcount is too small");
+ _PyObject_ASSERT(self,
+ (!_PyType_IS_GC(Py_TYPE(self))
+ || _PyObject_GC_IS_TRACKED(self)));
+
/* Undo the temporary resurrection; can't use DECREF here, it would
* cause a recursive call. */
- if (!_PyObject_ResurrectEnd(self)) {
- return 0; /* this is the normal path out */
+ if (_PyObject_ResurrectEnd(self)) {
+ /* tp_finalize resurrected it!
+ gh-130202: Note that the object may still be dead in the free
+ threaded build in some circumstances, so it's not safe to access
+ `self` after this point. For example, the last reference to the
+ resurrected `self` may be held by another thread, which can
+ concurrently deallocate it. */
+ return -1;
}
- /* tp_finalize resurrected it! Make it look like the original Py_DECREF
- * never happened. */
- _Py_ResurrectReference(self);
-
- _PyObject_ASSERT(self,
- (!_PyType_IS_GC(Py_TYPE(self))
- || _PyObject_GC_IS_TRACKED(self)));
- return -1;
+ /* this is the normal path out, the caller continues with deallocation. */
+ return 0;
}
int
@@ -2601,11 +2617,10 @@ _Py_ResurrectReference(PyObject *op)
#endif
}
-
-#ifdef Py_TRACE_REFS
void
_Py_ForgetReference(PyObject *op)
{
+#ifdef Py_TRACE_REFS
if (Py_REFCNT(op) < 0) {
_PyObject_ASSERT_FAILED_MSG(op, "negative refcnt");
}
@@ -2621,8 +2636,11 @@ _Py_ForgetReference(PyObject *op)
#endif
_PyRefchain_Remove(interp, op);
+#endif
}
+
+#ifdef Py_TRACE_REFS
static int
_Py_PrintReference(_Py_hashtable_t *ht,
const void *key, const void *value,
_______________________________________________
Python-checkins mailing list -- python-checkins@python.org
To unsubscribe send an email to python-checkins-le...@python.org
https://mail.python.org/mailman3/lists/python-checkins.python.org/
Member address: arch...@mail-archive.com
