Skip to content

gh-135552: Skip clearing of tp_subclasses weakrefs in GC #136147

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

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions Include/internal/pycore_interp_structs.h
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,7 @@ struct _Py_interp_static_objects {
_PyGC_Head_UNUSED _hamt_empty_gc_not_used;
PyHamtObject hamt_empty;
PyBaseExceptionObject last_resort_memory_error;
PyObject *subclasses_weakref_sentinel;
} singletons;
};

Expand Down
10 changes: 10 additions & 0 deletions Include/internal/pycore_weakref.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ extern "C" {
#endif

#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION()
#include "pycore_interp_structs.h" // PyInterpreterState
#include "pycore_lock.h" // PyMutex_LockFlags()
#include "pycore_object.h" // _Py_REF_IS_MERGED()
#include "pycore_pyatomic_ft_wrappers.h"
Expand Down Expand Up @@ -127,6 +128,15 @@ extern void _PyWeakref_ClearWeakRefsNoCallbacks(PyObject *obj);

PyAPI_FUNC(int) _PyWeakref_IsDead(PyObject *weakref);

// Create sentinel callback object for subclasses weakrefs
int _PyWeakref_InitSubclassSentinel(PyInterpreterState *interp);

// Create new PyWeakReference object with sentinel callback
PyObject * _PyWeakref_NewSubclassRef(PyObject *ob);

// Check if weakref has sentinel callback
int _PyWeakref_IsSubclassRef(PyWeakReference *weakref);

#ifdef __cplusplus
}
#endif
Expand Down
25 changes: 25 additions & 0 deletions Lib/test/test_gc.py
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,30 @@ def test_something(self):
""")
assert_python_ok("-c", source)

def test_do_not_cleanup_type_subclasses_before_finalization(self):
# https://github.com/python/cpython/issues/135552
code = """
class BaseNode:
def __del__(self):
BaseNode.next = BaseNode.next.next
BaseNode.next.next
class Node(BaseNode):
pass
BaseNode.next = Node()
BaseNode.next.next = Node()
"""
assert_python_ok("-c", textwrap.dedent(code))

code_inside_function = textwrap.dedent(F"""
def test():
{textwrap.indent(code, ' ')}
test()
""")
assert_python_ok("-c", code_inside_function)


class IncrementalGCTests(unittest.TestCase):
@unittest.skipIf(_testinternalcapi is None, "requires _testinternalcapi")
Expand Down Expand Up @@ -1518,6 +1542,7 @@ def test_ast_fini(self):
assert_python_ok("-c", code)



def setUpModule():
global enabled, debug
enabled = gc.isenabled()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Do not clear type's subclasses weak references in the GC to prevent the clearing
of the type's subclasses list too early. This fix a crash that occurs when
:meth:`~object.__del__` modifies the base's attributes and tries to access them
from self.
2 changes: 1 addition & 1 deletion Objects/typeobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -9259,7 +9259,7 @@ add_subclass(PyTypeObject *base, PyTypeObject *type)
if (key == NULL)
return -1;

PyObject *ref = PyWeakref_NewRef((PyObject *)type, NULL);
PyObject *ref = _PyWeakref_NewSubclassRef((PyObject *)type);
if (ref == NULL) {
Py_DECREF(key);
return -1;
Expand Down
54 changes: 54 additions & 0 deletions Objects/weakrefobject.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "Python.h"
#include "pycore_critical_section.h"
#include "pycore_global_objects.h" // _Py_INTERP_STATIC_OBJECT()
#include "pycore_interp_structs.h" // _PyInterpreterState_GET()
#include "pycore_lock.h"
#include "pycore_modsupport.h" // _PyArg_NoKwnames()
#include "pycore_object.h" // _PyObject_GET_WEAKREFS_LISTPTR()
Expand Down Expand Up @@ -1132,3 +1134,55 @@ _PyWeakref_IsDead(PyObject *weakref)
{
return _PyWeakref_IS_DEAD(weakref);
}

int
_PyWeakref_InitSubclassSentinel(PyInterpreterState *interp)
{
PyObject *code = (PyObject *)PyCode_NewEmpty(
"<generated>",
"<subclasses_weakref_sentinel>",
0);
if (code == NULL) {
return -1;
}

PyObject *globals = PyDict_New();
if (globals == NULL) {
Py_DECREF(code);
return -1;
}

PyObject *func = PyFunction_New(code, globals);
Py_DECREF(globals);
Py_DECREF(code);
if (func == NULL) {
return -1;
}

_Py_INTERP_SINGLETON(interp, subclasses_weakref_sentinel) = func;
return 0;
}

PyObject *
_PyWeakref_NewSubclassRef(PyObject *ob)
{
PyInterpreterState *interp = _PyInterpreterState_GET();
if (interp != interp->runtime->interpreters.main) {
interp = interp->runtime->interpreters.main;
}
PyObject *func = _Py_INTERP_SINGLETON(interp, subclasses_weakref_sentinel);
return PyWeakref_NewRef(ob, func);
}

int
_PyWeakref_IsSubclassRef(PyWeakReference *weakref)
{
assert(weakref != NULL);

PyInterpreterState *interp = _PyInterpreterState_GET();
if (interp != interp->runtime->interpreters.main) {
interp = interp->runtime->interpreters.main;
}
PyObject *func = _Py_INTERP_SINGLETON(interp, subclasses_weakref_sentinel);
return weakref->wr_callback == func;
}
22 changes: 20 additions & 2 deletions Python/gc.c
Original file line number Diff line number Diff line change
Expand Up @@ -906,8 +906,15 @@ handle_weakrefs(PyGC_Head *unreachable, PyGC_Head *old)
* reference cycle. If we don't clear the weakref, the callback
* will run and potentially cause a crash. See bpo-38006 for
* one example.
*
* If this is a subclass weakref we can safely ignore it's cleanup.
*/
_PyWeakref_ClearRef((PyWeakReference *)op);
if (!_PyWeakref_IsSubclassRef((PyWeakReference *)op)) {
_PyWeakref_ClearRef((PyWeakReference *)op);
}
else {
continue;
}
}

if (! _PyType_SUPPORTS_WEAKREFS(Py_TYPE(op))) {
Expand All @@ -925,9 +932,20 @@ handle_weakrefs(PyGC_Head *unreachable, PyGC_Head *old)
* all the weakrefs, and move the weakrefs with callbacks
* that must be called into wrcb_to_call.
*/
for (wr = *wrlist; wr != NULL; wr = *wrlist) {
PyWeakReference *wr_next = *wrlist;
for (wr = wr_next; wr != NULL; wr = wr_next) {
PyGC_Head *wrasgc; /* AS_GC(wr) */

wr_next = wr->wr_next;

/* If this is a subclass weakref we can safely ignore it's cleanup.
* It has only sentinel callback (no-op) and we also can safely
* not invoke them.
*/
if (_PyWeakref_IsSubclassRef(wr) == 1) {
continue;
}

/* _PyWeakref_ClearRef clears the weakref but leaves
* the callback pointer intact. Obscure: it also
* changes *wrlist.
Expand Down
6 changes: 6 additions & 0 deletions Python/pylifecycle.c
Original file line number Diff line number Diff line change
Expand Up @@ -1376,6 +1376,12 @@ init_interp_main(PyThreadState *tstate)
return _PyStatus_ERR("failed to set builtin dict watcher");
}

if (is_main_interp) {
if (_PyWeakref_InitSubclassSentinel(interp) < 0) {
return _PyStatus_ERR("failed to create subclasses weakref sentinel");
}
}

assert(!_PyErr_Occurred(tstate));

return _PyStatus_OK();
Expand Down
2 changes: 2 additions & 0 deletions Python/pystate.c
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,8 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)

Py_CLEAR(interp->audit_hooks);

Py_CLEAR(_Py_INTERP_SINGLETON(interp, subclasses_weakref_sentinel));

// At this time, all the threads should be cleared so we don't need atomic
// operations for instrumentation_version or eval_breaker.
interp->ceval.instrumentation_version = 0;
Expand Down
Loading