Skip to content
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 46 additions & 19 deletions Include/internal/pycore_weakref.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,49 +8,76 @@ extern "C" {
# error "this header requires Py_BUILD_CORE define"
#endif

#include "pycore_object.h" // _Py_REF_IS_MERGED()
#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION()

static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) {
assert(PyWeakref_Check(ref_obj));
PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj);
PyObject *obj = ref->wr_object;

if (obj == Py_None) {
// clear_weakref() was called
return NULL;
static inline int _is_dead(PyObject *obj)
{
if (obj == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check necessary? It doesn't look like obj should every be NULL because wr_object uses Py_None for dead objects instead of NULL.

return 0;
}

// Explanation for the Py_REFCNT() check: when a weakref's target is part
// of a long chain of deallocations which triggers the trashcan mechanism,
// clearing the weakrefs can be delayed long after the target's refcount
// has dropped to zero. In the meantime, code accessing the weakref will
// be able to "see" the target object even though it is supposed to be
// unreachable. See issue gh-60806.
Py_ssize_t refcnt = Py_REFCNT(obj);
if (refcnt == 0) {
return NULL;
#if defined(Py_GIL_DISABLED)
Py_ssize_t refcount = Py_REFCNT(obj);
if (refcount == 0) {
// In freethreading CPython, Py_REFCNT is only estimated count,
// so need to check shared refcount value too.
Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&obj->ob_ref_shared);
return _Py_REF_IS_MERGED(shared);
}
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit tricky. It's not enough to check that shared is merged -- the shared reference count must also be zero. The Py_REFCNT(obj) == 0 check may seem to imply this, but it doesn't both because it happens earlier in the code (and the reference count may change between the calls) and because the Py_REFCNT(obj) check itself is prone to race conditions since it needs to load the local and shared reference counts separately. That's why it's only an estimate.

It should be enough to only check the that the shared reference count is zero and that it is merged.

#else
return (Py_REFCNT(obj) == 0);
#endif
}

assert(refcnt > 0);
return Py_NewRef(obj);
static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj)
{
assert(PyWeakref_Check(ref_obj));
PyObject *ret = NULL;
Py_BEGIN_CRITICAL_SECTION(ref_obj);
PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj);
PyObject *obj = ref->wr_object;

if (obj == Py_None) {
// clear_weakref() was called
goto end;
}

if (_is_dead(obj)) {
goto end;
}
#if !defined(Py_GIL_DISABLED)
assert(Py_REFCNT(obj) > 0);
#endif
ret = Py_NewRef(obj);
end:
Py_END_CRITICAL_SECTION();
return ret;
}

static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) {
static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj)
{
assert(PyWeakref_Check(ref_obj));
int is_dead;
int ret = 0;
Py_BEGIN_CRITICAL_SECTION(ref_obj);
PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj);
PyObject *obj = ref->wr_object;
if (obj == Py_None) {
// clear_weakref() was called
is_dead = 1;
ret = 1;
}
else {
// See _PyWeakref_GET_REF() for the rationale of this test
Copy link
Contributor

@colesbury colesbury Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: removed suggestion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind -- that's probably not helpful. Can probably just delete the comment now that the code calls _is_dead(). The reader probably knows to look at the function implementation to understand the call.

is_dead = (Py_REFCNT(obj) == 0);
ret = _is_dead(obj);
}
Py_END_CRITICAL_SECTION();
return is_dead;
return ret;
}

extern Py_ssize_t _PyWeakref_GetWeakrefCount(PyWeakReference *head);
Expand Down