summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorSam Gross <colesbury@gmail.com>2024-12-05 21:07:31 (GMT)
committerGitHub <noreply@github.com>2024-12-05 21:07:31 (GMT)
commitf4f530804b9d8f089eba0f157ec2144c03b13651 (patch)
treece40d723b8c991d786924c16dd04dfa96ff8689b
parent657d0e99aa8754372786120d6ec00c9d9970e775 (diff)
downloadcpython-f4f530804b9d8f089eba0f157ec2144c03b13651.zip
cpython-f4f530804b9d8f089eba0f157ec2144c03b13651.tar.gz
cpython-f4f530804b9d8f089eba0f157ec2144c03b13651.tar.bz2
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.
-rw-r--r--Include/internal/pycore_object.h44
-rw-r--r--Misc/NEWS.d/next/Core_and_Builtins/2024-12-05-19-25-00.gh-issue-127582.ogUY2a.rst2
-rw-r--r--Objects/codeobject.c7
-rw-r--r--Objects/dictobject.c7
-rw-r--r--Objects/funcobject.c7
-rw-r--r--Objects/object.c40
6 files changed, 87 insertions, 20 deletions
diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h
index ce876b0..6b0b464 100644
--- a/Include/internal/pycore_object.h
+++ b/Include/internal/pycore_object.h
@@ -697,8 +697,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
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 148350c..eb8de13 100644
--- a/Objects/codeobject.c
+++ b/Objects/codeobject.c
@@ -1867,14 +1867,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 a13d808..1c9f864 100644
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -3162,14 +3162,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 4ba4728..cca7f01 100644
--- a/Objects/funcobject.c
+++ b/Objects/funcobject.c
@@ -1092,14 +1092,11 @@ static void
func_dealloc(PyObject *self)
{
PyFunctionObject *op = _PyFunction_CAST(self);
- assert(Py_REFCNT(op) == 0);
- Py_SET_REFCNT(op, 1);
+ _PyObject_ResurrectStart(self);
handle_func_event(PyFunction_EVENT_DESTROY, op, NULL);
- if (Py_REFCNT(op) > 1) {
- Py_SET_REFCNT(op, Py_REFCNT(op) - 1);
+ if (_PyObject_ResurrectEnd(self)) {
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 8868fa2..74f47fa 100644
--- a/Objects/object.c
+++ b/Objects/object.c
@@ -362,8 +362,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;
@@ -404,6 +406,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);
}
}
@@ -472,6 +483,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 */
@@ -550,7 +581,7 @@ PyObject_CallFinalizerFromDealloc(PyObject *self)
}
/* Temporarily resurrect the object. */
- Py_SET_REFCNT(self, 1);
+ _PyObject_ResurrectStart(self);
PyObject_CallFinalizer(self);
@@ -560,8 +591,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 */
}