Fix a reference leak of a non-Struct base's type dict - #1199
Conversation
sobolevn
left a comment
There was a problem hiding this comment.
Yes, this is correct :)
PyType_GetDict is defined as:
PyObject *
PyType_GetDict(PyTypeObject *self)
{
PyObject *dict = lookup_tp_dict(self);
return _Py_XNewRef(dict);
}So, the result if is not NULL must be decrefed in the end.
But, I think that we miss the == NULL check after the MS_GET_TYPE_DICT call as well. It can be NULL is come rare cases. We must not crash in this case.
CPython does check for NULL:
static int
PyCFuncPtrType_init(PyObject *self, PyObject *args, PyObject *kwds)
{
PyObject *attrdict = PyType_GetDict((PyTypeObject *)self);
if (!attrdict) {
return -1;
}And more note: the docs do not explicitly specify: when NULL can happen and if exception is set or not. This can be improved in the CPython repo.
sobolevn
left a comment
There was a problem hiding this comment.
Please, don't forget about NULL check :)
|
|
||
|
|
||
| @pytest.mark.parametrize("rejected", [False, True]) | ||
| def test_non_struct_base_type_dict_gains_no_references(rejected): |
There was a problem hiding this comment.
I advocate not to ever test refcounts :)
refcount is an implementation detail, it can change at almost any place in the future versions / build types.
Especially in such complex cases: a lot of things can go wrong. Types can get extra dict referents, like __annotations__ or something.
| assert sys.getrefcount(data) <= 4 | ||
|
|
||
|
|
||
| def test_struct_definition_does_not_leak_non_struct_base_dict(): |
There was a problem hiding this comment.
Removed the reference count test. I kept the rejected-base test as well, since it covers the release on the rejection exit, which this one does not.
A base that has not been readied yet, which a C extension can expose, has neither its type dict nor its inherited slots filled in, and `PyType_GetDict` returns NULL for it without setting an exception (before 3.12 the `tp_dict` slot is NULL), which crashed the `__init__` and `__new__` check. Ready such a base before reading its slots and dict, as type creation does for its bases, and raise `TypeError` if the dict is still missing. The check that the base is a type now comes before any of its slots are read, since only a type can be readied. Remove the reference count test; the two tests that check the base's contents stay.
|
Added the |
| info->already_has_dict = true; | ||
| } | ||
|
|
||
| if (!PyType_Check(base)) { |
There was a problem hiding this comment.
This is unrelated, but still does not look correct.
We first cast object to if ((PyTypeObject *)base and then check that it is a type.
Let's check that base is a type as the first step.
There was a problem hiding this comment.
Done: the type check is now the first step.
| * has neither its type dict nor its inherited slots filled in. Ready it | ||
| * before reading them, as type creation does for its bases. */ | ||
| if (!PyType_HasFeature((PyTypeObject *)base, Py_TPFLAGS_READY)) { | ||
| if (PyType_Ready((PyTypeObject *)base) < 0) return -1; |
There was a problem hiding this comment.
I don't think that it is correct to run PyType_Ready on types that we don't own. This can cause strange side-effects for users.
Just return -1 right away (with the exception set).
There was a problem hiding this comment.
Done: defining a Struct with an unready base now raises TypeError: Base class ... is not ready right away, and the base is left untouched.
Readying a type that belongs to another extension can have side effects, so raise TypeError for a base that is not ready yet. Check that the base is a type before anything else.
Entries for msgspec#1194, msgspec#1196, msgspec#1197, msgspec#1199 and msgspec#1209 are missing from the changelog of the 0.22.0 release. Of the changes merged after msgspec#1211 moved the unreleased entries into the 0.22.0 section, only msgspec#1207 added its own entry. This adds the missing entries next to the related entries in that section, and marks msgspec#1196 as a breaking change: objects that expose `__dataclass_fields__` only through instance attribute access are no longer encoded as dataclasses. Co-authored-by: Tseluiko Aleksandr <4410812+Siyet@users.noreply.github.com>
On Python 3.12+
MS_GET_TYPE_DICTexpands toPyType_GetDict, which returns a new reference; in the comment above the macro the result is described as borrowed. At its only call site, instructmeta_collect_base, there is no matching release on either exit, so defining aStructwith a non-Structbase leaks one reference to that base's type dict. Nothing holds that reference afterwards, so the dict is never freed, and on 3.12 through 3.14 that keeps the base class itself alive along with everything in its namespace, for the rest of the process.typing.Genericcounts as such a base, so genericStructdefinitions leak one too, in both the explicit and the PEP 695 spelling; there the dict stays reachable regardless, so only the count grows.Adds a release macro and applies it on both exits. It expands to
Py_XDECREFon 3.12+ and to nothing before that, where thetp_dictslot is read directly and the reference is borrowed. Corrects the comment to match. The remainingtp_dictuses in the file are direct slot reads guarded againstNULLand need no release.A base that has not been readied yet, which a C extension can expose, has no type dict, and on main the
__init__and__new__check crashes on such a base on every supported version. Such a base is now rejected withTypeErrorbefore its type dict or inherited slots are read, andTypeErroris also raised if a readied base still has no type dict. The check that the base is a type now comes first.Adds a regression test that the base's contents are released when the base passes the
__init__and__new__check. On current main it fails on 3.12, 3.13 and 3.14, and passes on 3.10 and 3.11, where the leak does not exist. It also passes on main on 3.15, where the leaked dict no longer keeps the base class reachable, so the class is collected and its contents are released even though the dict itself is still retained.Fixes #1198.