gh-130202: Fix bug in _PyObject_ResurrectEnd in free threaded build

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.

Author: colesbury

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

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