Skip to content

gh-95348: Split clear_weakref into two functions #95449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed
Changes from all 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
69 changes: 49 additions & 20 deletions Objects/weakrefobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,38 +46,56 @@ new_weakref(PyObject *ob, PyObject *callback)
}


/* This function clears the passed-in reference and removes it from the
* list of weak references for the referent. This is the only code that
* removes an item from the doubly-linked list of weak references for an
* object; it is also responsible for clearing the callback slot.
/* This function clears the passed-in reference. It must have been
* from the weaklist already. (See weaklist_remove_weakref().)
*/
static void
clear_weakref(PyWeakReference *self)
{
PyObject *callback = self->wr_callback;

if (self->wr_object != Py_None) {
PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object);
self->wr_object = Py_None;

if (*list == self)
/* If 'self' is the end of the list (and thus self->wr_next == NULL)
then the weakref list itself (and thus the value of *list) will
end up being set to NULL. */
*list = self->wr_next;
self->wr_object = Py_None;
Copy link
Member

Choose a reason for hiding this comment

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

Clearing wr_object should still happen in clear_weakref(), not remove_weakref().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is, the program segfaults when I reverse the order of remove before clear

weaklist_remove_weakref(list, *list);
clear_weakref(*list);

causes a segfault for me.

if (self->wr_prev != NULL)
self->wr_prev->wr_next = self->wr_next;
if (self->wr_next != NULL)
self->wr_next->wr_prev = self->wr_prev;
self->wr_prev = NULL;
self->wr_next = NULL;
}
if (callback != NULL) {
Py_DECREF(callback);
self->wr_callback = NULL;
}
}

/* This function removes the passed-in reference from the list of weak
* references for the referent.
*/
static void
weaklist_remove_weakref(PyWeakReference **list, PyWeakReference *ref)
{
if (*list == ref) {
/* If 'ref' is the end of the list (and thus ref->wr_next == NULL)
then the weakref list itself (and thus the value of *list) will
end up being set to NULL. */
*list = ref->wr_next;
}
if (ref->wr_prev != NULL) {
ref->wr_prev->wr_next = ref->wr_next;
}
if (ref->wr_next != NULL) {
ref->wr_next->wr_prev = ref->wr_prev;
}
ref->wr_prev = NULL;
ref->wr_next = NULL;
}

/* This function removes the pass in reference from the list of weak references
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* This function removes the pass in reference from the list of weak references
/* This function removes the passed-in reference from the list of weak references

* for the referent.
*/
static void
remove_weakref_from_referent(PyWeakReference *self)
{
if (self->wr_object != Py_None) {
PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object);
weaklist_remove_weakref(list, self);
}
}

/* Cyclic gc uses this to *just* clear the passed-in reference, leaving
* the callback intact and uncalled. It must be possible to call self's
* tp_dealloc() after calling this, so self has to be left in a sane enough
Expand All @@ -99,6 +117,7 @@ _PyWeakref_ClearRef(PyWeakReference *self)
/* Preserve and restore the callback around clear_weakref. */
callback = self->wr_callback;
self->wr_callback = NULL;
remove_weakref_from_referent(self);
clear_weakref(self);
self->wr_callback = callback;
}
Expand All @@ -107,6 +126,7 @@ static void
weakref_dealloc(PyObject *self)
{
PyObject_GC_UnTrack(self);
remove_weakref_from_referent((PyWeakReference *) self);
clear_weakref((PyWeakReference *) self);
Py_TYPE(self)->tp_free(self);
}
Expand All @@ -123,6 +143,7 @@ gc_traverse(PyWeakReference *self, visitproc visit, void *arg)
static int
gc_clear(PyWeakReference *self)
{
remove_weakref_from_referent(self);
clear_weakref(self);
return 0;
}
Expand Down Expand Up @@ -562,6 +583,7 @@ proxy_dealloc(PyWeakReference *self)
if (self->wr_callback != NULL)
PyObject_GC_UnTrack((PyObject *)self);
clear_weakref(self);
remove_weakref_from_referent(self);
PyObject_GC_Del(self);
}

Expand Down Expand Up @@ -958,8 +980,11 @@ PyObject_ClearWeakRefs(PyObject *object)
/* Remove the callback-less basic and proxy references */
if (*list != NULL && (*list)->wr_callback == NULL) {
clear_weakref(*list);
if (*list != NULL && (*list)->wr_callback == NULL)
weaklist_remove_weakref(list, *list);
if (*list != NULL && (*list)->wr_callback == NULL) {
clear_weakref(*list);
weaklist_remove_weakref(list, *list);
}
}
if (*list != NULL) {
PyWeakReference *current = *list;
Expand All @@ -972,6 +997,7 @@ PyObject_ClearWeakRefs(PyObject *object)

current->wr_callback = NULL;
clear_weakref(current);
weaklist_remove_weakref(list, current);
if (callback != NULL) {
if (Py_REFCNT((PyObject *)current) > 0) {
handle_callback(current, callback);
Expand Down Expand Up @@ -1002,6 +1028,7 @@ PyObject_ClearWeakRefs(PyObject *object)
}
current->wr_callback = NULL;
clear_weakref(current);
weaklist_remove_weakref(list, current);
current = next;
}
for (i = 0; i < count; ++i) {
Expand Down Expand Up @@ -1036,5 +1063,7 @@ _PyStaticType_ClearWeakRefs(PyTypeObject *type)
weaklist before clearing its wr_object and wr_callback.
That is how we're able to loop over the list. */
clear_weakref((PyWeakReference *)*list);
weaklist_remove_weakref((PyWeakReference **)list,
(PyWeakReference *)list);
}
}