From cc0f56cea8e7d4fdd75f5929bd6637d3401efdd1 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 20 Jul 2022 15:46:31 -0600 Subject: [PATCH 1/6] Add a note about skipping on subclasses. --- Objects/typeobject.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 5ebff6084f4a97..ae3a50372b35f3 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -4214,9 +4214,16 @@ type_dealloc_common(PyTypeObject *type) void _PyStaticType_Dealloc(PyTypeObject *type) { - // If a type still has subtypes, it cannot be deallocated. - // A subtype can inherit attributes and methods of its parent type, - // and a type must no longer be used once it's deallocated. + /* At this point in the runtime lifecycle, if a type still has + subtypes then some extension module did not correctly finalize + its objects. We can ignore such sybtypes since such extension + modules are already unsafe if the runtime is re-used after + finalization (or in multiple interpreters). + + Unfortunately, this means we will leak the objects for which + the subtype owns a reference (directly or indirectly). */ + // XXX For now we abandon finalizing the static type here and + // instead leak the type's objects. if (type->tp_subclasses != NULL) { return; } From 34c5d3b2e8b2a4aaf1264356e092b21ad04f08c7 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 20 Jul 2022 17:59:42 -0600 Subject: [PATCH 2/6] Throw away the leaking subclasses. --- Objects/typeobject.c | 46 ++++++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index ae3a50372b35f3..ca7b0e6ddc3ef3 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -4211,30 +4211,48 @@ type_dealloc_common(PyTypeObject *type) } -void -_PyStaticType_Dealloc(PyTypeObject *type) +static void +clear_static_tp_subclasses(PyTypeObject *type) { - /* At this point in the runtime lifecycle, if a type still has - subtypes then some extension module did not correctly finalize - its objects. We can ignore such sybtypes since such extension - modules are already unsafe if the runtime is re-used after - finalization (or in multiple interpreters). - - Unfortunately, this means we will leak the objects for which - the subtype owns a reference (directly or indirectly). */ - // XXX For now we abandon finalizing the static type here and - // instead leak the type's objects. - if (type->tp_subclasses != NULL) { + if (type->tp_subclasses == NULL) { return; } + /* Normally it would be a problem to finalize the type if its + tp_subclasses wasn't cleared first. However, this is only + ever called at the end of runtime finalization, so we can be + more liberal in cleaning up. If the given type still has + subtypes at this point then some extension module did not + correctly finalize its objects. + + We can safely obliterate such sybtypes since the extension + module and its objects won't be used again, except maybe if + the runtime were re-initialized. In that case the sticky + situation would only happen if the module were re-imported + then and only if the subtype were stored in a global and only + if that global were not overwritten during import. We'd be + fine since the extension is otherwise unsafe and unsupported + in that situation, and likely problematic already. + + In any case, this situation means at least some memory is + going to leak. This mostly only affects embedding scenarios. + */ + + // For now we just clear tp_subclasses. + + Py_CLEAR(type->tp_subclasses); +} + +void +_PyStaticType_Dealloc(PyTypeObject *type) +{ type_dealloc_common(type); Py_CLEAR(type->tp_dict); Py_CLEAR(type->tp_bases); Py_CLEAR(type->tp_mro); Py_CLEAR(type->tp_cache); - // type->tp_subclasses is NULL + clear_static_tp_subclasses(type); // PyObject_ClearWeakRefs() raises an exception if Py_REFCNT() != 0 if (Py_REFCNT(type) == 0) { From 8d64d72326a0633895e783d7a0d42e334b6750bf Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 22 Jul 2022 14:50:29 -0600 Subject: [PATCH 3/6] Factor out subclass_from_ref(). --- Objects/typeobject.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index ca7b0e6ddc3ef3..d79c37bf5380a6 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -65,6 +65,8 @@ lookup_maybe_method(PyObject *self, PyObject *attr, int *unbound); static int slot_tp_setattro(PyObject *self, PyObject *name, PyObject *value); +static inline PyTypeObject * subclass_from_ref(PyObject *ref); + /* * finds the beginning of the docstring's introspection signature. * if present, returns a pointer pointing to the first '('. @@ -309,12 +311,11 @@ PyType_Modified(PyTypeObject *type) Py_ssize_t i = 0; PyObject *ref; while (PyDict_Next(subclasses, &i, NULL, &ref)) { - assert(PyWeakref_CheckRef(ref)); - PyObject *obj = PyWeakref_GET_OBJECT(ref); - if (obj == Py_None) { + PyTypeObject *subclass = subclass_from_ref(ref); // borrowed + if (subclass == NULL) { continue; } - PyType_Modified(_PyType_CAST(obj)); + PyType_Modified(subclass); } } @@ -4321,14 +4322,12 @@ _PyType_GetSubclasses(PyTypeObject *self) Py_ssize_t i = 0; PyObject *ref; // borrowed ref while (PyDict_Next(subclasses, &i, NULL, &ref)) { - assert(PyWeakref_CheckRef(ref)); - PyObject *obj = PyWeakref_GET_OBJECT(ref); // borrowed ref - if (obj == Py_None) { + PyTypeObject *subclass = subclass_from_ref(ref); // borrowed + if (subclass == NULL) { continue; } - assert(PyType_Check(obj)); - if (PyList_Append(list, obj) < 0) { + if (PyList_Append(list, _PyObject_CAST(subclass)) < 0) { Py_DECREF(list); return NULL; } @@ -6725,6 +6724,19 @@ add_all_subclasses(PyTypeObject *type, PyObject *bases) return res; } +static inline PyTypeObject * +subclass_from_ref(PyObject *ref) +{ + assert(PyWeakref_CheckRef(ref)); + PyObject *obj = PyWeakref_GET_OBJECT(ref); // borrowed ref + assert(obj != NULL); + if (obj == Py_None) { + return NULL; + } + assert(PyType_Check(obj)); + return _PyType_CAST(obj); +} + static void remove_subclass(PyTypeObject *base, PyTypeObject *type) { @@ -8828,13 +8840,10 @@ recurse_down_subclasses(PyTypeObject *type, PyObject *attr_name, Py_ssize_t i = 0; PyObject *ref; while (PyDict_Next(subclasses, &i, NULL, &ref)) { - assert(PyWeakref_CheckRef(ref)); - PyObject *obj = PyWeakref_GET_OBJECT(ref); - assert(obj != NULL); - if (obj == Py_None) { + PyTypeObject *subclass = subclass_from_ref(ref); // borrowed + if (subclass == NULL) { continue; } - PyTypeObject *subclass = _PyType_CAST(obj); /* Avoid recursing down into unaffected classes */ PyObject *dict = subclass->tp_dict; From fe8cce9637107056640580726ed6b933bd17bc8a Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 22 Jul 2022 15:29:16 -0600 Subject: [PATCH 4/6] Handle MemoryError in remove_subclass(). --- Objects/typeobject.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index d79c37bf5380a6..9f335041fd690d 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -6737,6 +6737,29 @@ subclass_from_ref(PyObject *ref) return _PyType_CAST(obj); } +static PyObject * +get_subclasses_key(PyTypeObject *type, PyTypeObject *base) +{ + PyObject *key = PyLong_FromVoidPtr((void *) type); + if (key != NULL) { + return key; + } + PyErr_Clear(); + + /* This basically means we're out of memory. + We fall back to manually traversing the values. */ + Py_ssize_t i = 0; + PyObject *ref; // borrowed ref + while (PyDict_Next((PyObject *)base->tp_subclasses, &i, &key, &ref)) { + PyTypeObject *subclass = subclass_from_ref(ref); // borrowed + if (subclass == type) { + return Py_NewRef(key); + } + } + /* It wasn't found. */ + return NULL; +} + static void remove_subclass(PyTypeObject *base, PyTypeObject *type) { @@ -6746,8 +6769,8 @@ remove_subclass(PyTypeObject *base, PyTypeObject *type) } assert(PyDict_CheckExact(subclasses)); - PyObject *key = PyLong_FromVoidPtr((void *) type); - if (key == NULL || PyDict_DelItem(subclasses, key)) { + PyObject *key = get_subclasses_key(type, base); + if (key != NULL && PyDict_DelItem(subclasses, key)) { /* This can happen if the type initialization errored out before the base subclasses were updated (e.g. a non-str __qualname__ was passed in the type dict). */ From 74d5af3146e4567791c12df213978ceabd5a075c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 22 Jul 2022 15:44:36 -0600 Subject: [PATCH 5/6] Call _PyTypes_FiniTypes() before _PyTypes_Fini(). --- Python/pylifecycle.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 65e7f23e963b5f..75aba4a25c41a9 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1672,9 +1672,10 @@ finalize_interp_types(PyInterpreterState *interp) _PyLong_FiniTypes(interp); _PyThread_FiniType(interp); _PyErr_FiniTypes(interp); - _PyTypes_Fini(interp); _PyTypes_FiniTypes(interp); + _PyTypes_Fini(interp); + // Call _PyUnicode_ClearInterned() before _PyDict_Fini() since it uses // a dict internally. _PyUnicode_ClearInterned(interp); From d91a0d68a0ca4677b4db256448bc93ae37b1e0c4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 25 Jul 2022 12:15:51 -0600 Subject: [PATCH 6/6] Update Objects/typeobject.c Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> --- Objects/typeobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 9f335041fd690d..8dc18eeee14437 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -4226,7 +4226,7 @@ clear_static_tp_subclasses(PyTypeObject *type) subtypes at this point then some extension module did not correctly finalize its objects. - We can safely obliterate such sybtypes since the extension + We can safely obliterate such subtypes since the extension module and its objects won't be used again, except maybe if the runtime were re-initialized. In that case the sticky situation would only happen if the module were re-imported