Skip to content

Commit

Permalink
[3.13] pythongh-127582: Make object resurrection thread-safe for free…
Browse files Browse the repository at this point in the history
… threading. (pythonGH-127612) (pythonGH-127659)

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 f4f5308)
  • Loading branch information
colesbury authored Dec 5, 2024
1 parent 304111e commit 69bb1c6
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 20 deletions.
44 changes: 44 additions & 0 deletions Include/internal/pycore_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix non-thread-safe object resurrection when calling finalizers and watcher
callbacks in the free threading build.
7 changes: 2 additions & 5 deletions Objects/codeobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
7 changes: 2 additions & 5 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 2 additions & 5 deletions Objects/funcobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
40 changes: 35 additions & 5 deletions Objects/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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 */


Expand Down Expand Up @@ -548,7 +579,7 @@ PyObject_CallFinalizerFromDealloc(PyObject *self)
}

/* Temporarily resurrect the object. */
Py_SET_REFCNT(self, 1);
_PyObject_ResurrectStart(self);

PyObject_CallFinalizer(self);

Expand All @@ -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 */
}

Expand Down

0 comments on commit 69bb1c6

Please sign in to comment.