[3.13] gh-127582: Make object resurrection thread-safe for free threading. (GH-127612...

Objects may be temporarily "resurrected" in destructors when calling
finalizers or watcher callbacks. We previously undid the resurrection
by decrementing the reference count using `Py_SET_REFCNT`. This was not
thread-safe because other threads might be accessing the object
(modifying its reference count) if it was exposed by the finalizer,
watcher callback, or temporarily accessed by a racy dictionary or list
access.

This adds internal-only thread-safe functions for temporary object
resurrection during destructors.
(cherry picked from commit f4f530804b9d8f089eba0f157ec2144c03b13651)

diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h
index d50a688d5b752a6bc55e5263c8f59c53d1ea6fac..800e427bd719d7a57f8fa81b76ca919ebad9240a 100644 (file)
--- a/Include/internal/pycore_object.h
+++ b/Include/internal/pycore_object.h
@@ -572,8 +572,52 @@ _PyObject_SetMaybeWeakref(PyObject *op)
     }
 }
 
+extern int _PyObject_ResurrectEndSlow(PyObject *op);
 #endif
 
+// Temporarily resurrects an object during deallocation. The refcount is set
+// to one.
+static inline void
+_PyObject_ResurrectStart(PyObject *op)
+{
+    assert(Py_REFCNT(op) == 0);
+#ifdef Py_REF_DEBUG
+    _Py_IncRefTotal(_PyThreadState_GET());
+#endif
+#ifdef Py_GIL_DISABLED
+    _Py_atomic_store_uintptr_relaxed(&op->ob_tid, _Py_ThreadId());
+    _Py_atomic_store_uint32_relaxed(&op->ob_ref_local, 1);
+    _Py_atomic_store_ssize_relaxed(&op->ob_ref_shared, 0);
+#else
+    Py_SET_REFCNT(op, 1);
+#endif
+}
+
+// Undoes an object resurrection by decrementing the refcount without calling
+// _Py_Dealloc(). Returns 0 if the object is dead (the normal case), and
+// deallocation should continue. Returns 1 if the object is still alive.
+static inline int
+_PyObject_ResurrectEnd(PyObject *op)
+{
+#ifdef Py_REF_DEBUG
+    _Py_DecRefTotal(_PyThreadState_GET());
+#endif
+#ifndef Py_GIL_DISABLED
+    Py_SET_REFCNT(op, Py_REFCNT(op) - 1);
+    return Py_REFCNT(op) != 0;
+#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);
+        return 0;
+    }
+    // Slow-path: object has a shared refcount or is not owned by this thread
+    return _PyObject_ResurrectEndSlow(op);
+#endif
+}
+
 /* Tries to incref op and returns 1 if successful or 0 otherwise. */
 static inline int
 _Py_TryIncref(PyObject *op)

diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-12-05-19-25-00.gh-issue-127582.ogUY2a.rst b/Misc/NEWS.d/next/Core and Builtins/2024-12-05-19-25-00.gh-issue-127582.ogUY2a.rst
new file mode 100644 (file)
index 0000000..59491fe
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2024-12-05-19-25-00.gh-issue-127582.ogUY2a.rst
@@ -0,0 +1,2 @@
+Fix non-thread-safe object resurrection when calling finalizers and watcher
+callbacks in the free threading build.

diff --git a/Objects/codeobject.c b/Objects/codeobject.c
index 83b477e19c7a5bbe222f56c11ffc016ea86d82ca..6c4eef8e0116a3c00e14f1e604dbd172b11d7b31 100644 (file)
--- a/Objects/codeobject.c
+++ b/Objects/codeobject.c
@@ -1845,14 +1845,11 @@ free_monitoring_data(_PyCoMonitoringData *data)
 static void
 code_dealloc(PyCodeObject *co)
 {
-    assert(Py_REFCNT(co) == 0);
-    Py_SET_REFCNT(co, 1);
+    _PyObject_ResurrectStart((PyObject *)co);
     notify_code_watchers(PY_CODE_EVENT_DESTROY, co);
-    if (Py_REFCNT(co) > 1) {
-        Py_SET_REFCNT(co, Py_REFCNT(co) - 1);
+    if (_PyObject_ResurrectEnd((PyObject *)co)) {
         return;
     }
-    Py_SET_REFCNT(co, 0);
 
 #ifdef Py_GIL_DISABLED
     PyObject_GC_UnTrack(co);

diff --git a/Objects/dictobject.c b/Objects/dictobject.c
index f1f9110ff73e6d148bf9b95109ce5d8beecf8f1b..e82cd2d308ca7839073a388d52a185296f021850 100644 (file)
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -3150,14 +3150,11 @@ dict_dealloc(PyObject *self)
 {
     PyDictObject *mp = (PyDictObject *)self;
     PyInterpreterState *interp = _PyInterpreterState_GET();
-    assert(Py_REFCNT(mp) == 0);
-    Py_SET_REFCNT(mp, 1);
+    _PyObject_ResurrectStart(self);
     _PyDict_NotifyEvent(interp, PyDict_EVENT_DEALLOCATED, mp, NULL, NULL);
-    if (Py_REFCNT(mp) > 1) {
-        Py_SET_REFCNT(mp, Py_REFCNT(mp) - 1);
+    if (_PyObject_ResurrectEnd(self)) {
         return;
     }
-    Py_SET_REFCNT(mp, 0);
     PyDictValues *values = mp->ma_values;
     PyDictKeysObject *keys = mp->ma_keys;
     Py_ssize_t i, n;

diff --git a/Objects/funcobject.c b/Objects/funcobject.c
index 8a30213888ef870f4c36907574bd1d45f7129e11..12d60f991534ab5fc7a52c64af2b36d382f50fa6 100644 (file)
--- a/Objects/funcobject.c
+++ b/Objects/funcobject.c
@@ -986,14 +986,11 @@ func_clear(PyFunctionObject *op)
 static void
 func_dealloc(PyFunctionObject *op)
 {
-    assert(Py_REFCNT(op) == 0);
-    Py_SET_REFCNT(op, 1);
+    _PyObject_ResurrectStart((PyObject *)op);
     handle_func_event(PyFunction_EVENT_DESTROY, op, NULL);
-    if (Py_REFCNT(op) > 1) {
-        Py_SET_REFCNT(op, Py_REFCNT(op) - 1);
+    if (_PyObject_ResurrectEnd((PyObject *)op)) {
         return;
     }
-    Py_SET_REFCNT(op, 0);
     _PyObject_GC_UNTRACK(op);
     if (op->func_weakreflist != NULL) {
         PyObject_ClearWeakRefs((PyObject *) op);

diff --git a/Objects/object.c b/Objects/object.c
index a03c0cc55b4ae2c5e4dbcb245067993569e1442a..bfdd10faa2b638fe24c70aae9a3694405eadfc20 100644 (file)
--- a/Objects/object.c
+++ b/Objects/object.c
@@ -360,8 +360,10 @@ is_dead(PyObject *o)
 }
 # endif
 
-void
-_Py_DecRefSharedDebug(PyObject *o, const char *filename, int lineno)
+// Decrement the shared reference count of an object. Return 1 if the object
+// is dead and should be deallocated, 0 otherwise.
+static int
+_Py_DecRefSharedIsDead(PyObject *o, const char *filename, int lineno)
 {
     // Should we queue the object for the owning thread to merge?
     int should_queue;
@@ -402,6 +404,15 @@ _Py_DecRefSharedDebug(PyObject *o, const char *filename, int lineno)
     }
     else if (new_shared == _Py_REF_MERGED) {
         // refcount is zero AND merged
+        return 1;
+    }
+    return 0;
+}
+
+void
+_Py_DecRefSharedDebug(PyObject *o, const char *filename, int lineno)
+{
+    if (_Py_DecRefSharedIsDead(o, filename, lineno)) {
         _Py_Dealloc(o);
     }
 }
@@ -470,6 +481,26 @@ _Py_ExplicitMergeRefcount(PyObject *op, Py_ssize_t extra)
                                                 &shared, new_shared));
     return refcnt;
 }
+
+// The more complicated "slow" path for undoing the resurrection of an object.
+int
+_PyObject_ResurrectEndSlow(PyObject *op)
+{
+    if (_Py_IsImmortal(op)) {
+        return 1;
+    }
+    if (_Py_IsOwnedByCurrentThread(op)) {
+        // If the object is owned by the current thread, give up ownership and
+        // 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;
+    }
+    int is_dead = _Py_DecRefSharedIsDead(op, NULL, 0);
+    return !is_dead;
+}
+
+
 #endif  /* Py_GIL_DISABLED */
 
 
@@ -548,7 +579,7 @@ PyObject_CallFinalizerFromDealloc(PyObject *self)
     }
 
     /* Temporarily resurrect the object. */
-    Py_SET_REFCNT(self, 1);
+    _PyObject_ResurrectStart(self);
 
     PyObject_CallFinalizer(self);
 
@@ -558,8 +589,7 @@ PyObject_CallFinalizerFromDealloc(PyObject *self)
 
     /* Undo the temporary resurrection; can't use DECREF here, it would
      * cause a recursive call. */
-    Py_SET_REFCNT(self, Py_REFCNT(self) - 1);
-    if (Py_REFCNT(self) == 0) {
+    if (!_PyObject_ResurrectEnd(self)) {
         return 0;         /* this is the normal path out */
     }