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.

diff --git a/Include/cpython/object.h b/Include/cpython/object.h
index 260b90da24c18b6ebeea93206df168346a9e2ff9..f466091e07e465c116ba7051544b5b6552eafdbe 100644 (file)
--- 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 ffd31bd4a27f498582b04da89fab8b12ee077fe9..53403ebcfc00439bad15ece28835a2585d1be062 100644 (file)
--- 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 d342549b6ffecc8f70caec0c88a88a90f40b65ed..b3309bac7afdeed5303eae50c7e2b9bcbee25831 100644 (file)
--- 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,