From 1cf15af9a6f28750f37b08c028ada31d38e818dd Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Wed, 27 May 2020 10:03:38 +0100 Subject: [PATCH] bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types (reverts GH-19414) (GH-20264) Heap types now always visit the type in tp_traverse. See added docs for details. This reverts commit 0169d3003be3d072751dd14a5c84748ab63a249f. Automerge-Triggered-By: @encukou --- Doc/c-api/typeobj.rst | 16 ++- Doc/whatsnew/3.9.rst | 49 +++++++++ .../2020-05-23-01-15-51.bpo-40217.jZsHTc.rst | 4 + Modules/_abc.c | 1 + Modules/_curses_panel.c | 1 + Modules/_json.c | 2 + Modules/_struct.c | 1 + Modules/xxlimited.c | 1 + Objects/structseq.c | 3 + Objects/typeobject.c | 101 ++---------------- Parser/asdl_c.py | 1 + Python/Python-ast.c | 1 + 12 files changed, 87 insertions(+), 94 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2020-05-23-01-15-51.bpo-40217.jZsHTc.rst diff --git a/Doc/c-api/typeobj.rst b/Doc/c-api/typeobj.rst index ce4e8c926b2..385c7f94c67 100644 --- a/Doc/c-api/typeobj.rst +++ b/Doc/c-api/typeobj.rst @@ -1223,11 +1223,25 @@ and :c:type:`PyType_Type` effectively act as defaults.) but the instance has no strong reference to the elements inside it, as they are allowed to be removed even if the instance is still alive). - Note that :c:func:`Py_VISIT` requires the *visit* and *arg* parameters to :c:func:`local_traverse` to have these specific names; don't name them just anything. + Heap-allocated types (:const:`Py_TPFLAGS_HEAPTYPE`, such as those created + with :c:func:`PyType_FromSpec` and similar APIs) hold a reference to their + type. Their traversal function must therefore either visit + :c:func:`Py_TYPE(self) `, or delegate this responsibility by + calling ``tp_traverse`` of another heap-allocated type (such as a + heap-allocated superclass). + If they do not, the type object may not be garbage-collected. + + .. versionchanged:: 3.9 + + Heap-allocated types are expected to visit ``Py_TYPE(self)`` in + ``tp_traverse``. In earlier versions of Python, due to + `bug 40217 `_, doing this + may lead to crashes in subclasses. + **Inheritance:** Group: :const:`Py_TPFLAGS_HAVE_GC`, :attr:`tp_traverse`, :attr:`tp_clear` diff --git a/Doc/whatsnew/3.9.rst b/Doc/whatsnew/3.9.rst index d72fea2c679..8a04f725133 100644 --- a/Doc/whatsnew/3.9.rst +++ b/Doc/whatsnew/3.9.rst @@ -933,6 +933,55 @@ Changes in the Python API (Contributed by Inada Naoki in :issue:`34538`.) +Changes in the C API +-------------------- + +* Instances of heap-allocated types (such as those created with + :c:func:`PyType_FromSpec` and similar APIs) hold a reference to their type + object since Python 3.8. As indicated in the "Changes in the C API" of Python + 3.8, for the vast majority of cases, there should be no side effect but for + types that have a custom :c:member:`~PyTypeObject.tp_traverse` function, + ensure that all custom ``tp_traverse`` functions of heap-allocated types + visit the object's type. + + Example: + + .. code-block:: c + + int + foo_traverse(foo_struct *self, visitproc visit, void *arg) { + // Rest of the traverse function + #if PY_VERSION_HEX >= 0x03090000 + // This was not needed before Python 3.9 (Python issue 35810 and 40217) + Py_VISIT(Py_TYPE(self)); + #endif + } + + If your traverse function delegates to ``tp_traverse`` of its base class + (or another type), ensure that ``Py_TYPE(self)`` is visited only once. + Note that only heap types are expected to visit the type in ``tp_traverse``. + + For example, if your ``tp_traverse`` function includes: + + .. code-block:: c + + base->tp_traverse(self, visit, arg) + + then add: + + .. code-block:: c + + #if PY_VERSION_HEX >= 0x03090000 + // This was not needed before Python 3.9 (Python issue 35810 and 40217) + if (base->tp_flags & Py_TPFLAGS_HEAPTYPE) { + // a heap type's tp_traverse already visited Py_TYPE(self) + } else { + Py_VISIT(Py_TYPE(self)); + } + #else + + (See :issue:`35810` and :issue:`40217` for more information.) + CPython bytecode changes ------------------------ diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-05-23-01-15-51.bpo-40217.jZsHTc.rst b/Misc/NEWS.d/next/Core and Builtins/2020-05-23-01-15-51.bpo-40217.jZsHTc.rst new file mode 100644 index 00000000000..b13e8eeb063 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2020-05-23-01-15-51.bpo-40217.jZsHTc.rst @@ -0,0 +1,4 @@ +Instances of types created with :c:func:`PyType_FromSpecWithBases` will no +longer automatically visit their class object when traversing references in +the garbage collector. The user is expected to manually visit the object's +class. Patch by Pablo Galindo. diff --git a/Modules/_abc.c b/Modules/_abc.c index 434bc454175..709b52ff96b 100644 --- a/Modules/_abc.c +++ b/Modules/_abc.c @@ -46,6 +46,7 @@ typedef struct { static int abc_data_traverse(_abc_data *self, visitproc visit, void *arg) { + Py_VISIT(Py_TYPE(self)); Py_VISIT(self->_abc_registry); Py_VISIT(self->_abc_cache); Py_VISIT(self->_abc_negative_cache); diff --git a/Modules/_curses_panel.c b/Modules/_curses_panel.c index 7ca91f64161..f124803493d 100644 --- a/Modules/_curses_panel.c +++ b/Modules/_curses_panel.c @@ -39,6 +39,7 @@ _curses_panel_clear(PyObject *m) static int _curses_panel_traverse(PyObject *m, visitproc visit, void *arg) { + Py_VISIT(Py_TYPE(m)); Py_VISIT(get_curses_panelstate(m)->PyCursesError); return 0; } diff --git a/Modules/_json.c b/Modules/_json.c index 075aa3d2f4f..faa3944eedd 100644 --- a/Modules/_json.c +++ b/Modules/_json.c @@ -647,6 +647,7 @@ scanner_dealloc(PyObject *self) static int scanner_traverse(PyScannerObject *self, visitproc visit, void *arg) { + Py_VISIT(Py_TYPE(self)); Py_VISIT(self->object_hook); Py_VISIT(self->object_pairs_hook); Py_VISIT(self->parse_float); @@ -1745,6 +1746,7 @@ encoder_dealloc(PyObject *self) static int encoder_traverse(PyEncoderObject *self, visitproc visit, void *arg) { + Py_VISIT(Py_TYPE(self)); Py_VISIT(self->markers); Py_VISIT(self->defaultfn); Py_VISIT(self->encoder); diff --git a/Modules/_struct.c b/Modules/_struct.c index 5984bb68114..f759f0b1694 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -1646,6 +1646,7 @@ unpackiter_dealloc(unpackiterobject *self) static int unpackiter_traverse(unpackiterobject *self, visitproc visit, void *arg) { + Py_VISIT(Py_TYPE(self)); Py_VISIT(self->so); Py_VISIT(self->buf.obj); return 0; diff --git a/Modules/xxlimited.c b/Modules/xxlimited.c index 7ce0b6ec880..5b05a9454a0 100644 --- a/Modules/xxlimited.c +++ b/Modules/xxlimited.c @@ -43,6 +43,7 @@ newXxoObject(PyObject *arg) static int Xxo_traverse(XxoObject *self, visitproc visit, void *arg) { + Py_VISIT(Py_TYPE(self)); Py_VISIT(self->x_attr); return 0; } diff --git a/Objects/structseq.c b/Objects/structseq.c index 9bdda87ae0b..b17b1f99a5b 100644 --- a/Objects/structseq.c +++ b/Objects/structseq.c @@ -70,6 +70,9 @@ PyStructSequence_GetItem(PyObject* op, Py_ssize_t i) static int structseq_traverse(PyStructSequence *obj, visitproc visit, void *arg) { + if (Py_TYPE(obj)->tp_flags & Py_TPFLAGS_HEAPTYPE) { + Py_VISIT(Py_TYPE(obj)); + } Py_ssize_t i, size; size = REAL_SIZE(obj); for (i = 0; i < size; ++i) { diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 0e055d677f1..ba2a852cdda 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -1039,42 +1039,6 @@ type_call(PyTypeObject *type, PyObject *args, PyObject *kwds) return obj; } -PyObject * -PyType_FromSpec_Alloc(PyTypeObject *type, Py_ssize_t nitems) -{ - PyObject *obj; - const size_t size = _Py_SIZE_ROUND_UP( - _PyObject_VAR_SIZE(type, nitems+1) + sizeof(traverseproc), - SIZEOF_VOID_P); - /* note that we need to add one, for the sentinel and space for the - provided tp-traverse: See bpo-40217 for more details */ - - if (PyType_IS_GC(type)) { - obj = _PyObject_GC_Malloc(size); - } - else { - obj = (PyObject *)PyObject_MALLOC(size); - } - - if (obj == NULL) { - return PyErr_NoMemory(); - } - - memset(obj, '\0', size); - - if (type->tp_itemsize == 0) { - (void)PyObject_INIT(obj, type); - } - else { - (void) PyObject_INIT_VAR((PyVarObject *)obj, type, nitems); - } - - if (PyType_IS_GC(type)) { - _PyObject_GC_TRACK(obj); - } - return obj; -} - PyObject * PyType_GenericAlloc(PyTypeObject *type, Py_ssize_t nitems) { @@ -1164,11 +1128,16 @@ subtype_traverse(PyObject *self, visitproc visit, void *arg) Py_VISIT(*dictptr); } - if (type->tp_flags & Py_TPFLAGS_HEAPTYPE) + if (type->tp_flags & Py_TPFLAGS_HEAPTYPE + && (!basetraverse || !(base->tp_flags & Py_TPFLAGS_HEAPTYPE))) { /* For a heaptype, the instances count as references to the type. Traverse the type so the collector - can find cycles involving this link. */ + can find cycles involving this link. + Skip this visit if basetraverse belongs to a heap type: in that + case, basetraverse will visit the type when we call it later. + */ Py_VISIT(type); + } if (basetraverse) return basetraverse(self, visit, arg); @@ -2910,36 +2879,6 @@ static const short slotoffsets[] = { #include "typeslots.inc" }; -static int -PyType_FromSpec_tp_traverse(PyObject *self, visitproc visit, void *arg) -{ - PyTypeObject *parent = Py_TYPE(self); - - // Only a instance of a type that is directly created by - // PyType_FromSpec (not subclasses) must visit its parent. - if (parent->tp_traverse == PyType_FromSpec_tp_traverse) { - Py_VISIT(parent); - } - - // Search for the original type that was created using PyType_FromSpec - PyTypeObject *base; - base = parent; - while (base->tp_traverse != PyType_FromSpec_tp_traverse) { - base = base->tp_base; - assert(base); - } - - // Extract the user defined traverse function that we placed at the end - // of the type and call it. - size_t size = Py_SIZE(base); - size_t _offset = _PyObject_VAR_SIZE(&PyType_Type, size+1); - traverseproc fun = *(traverseproc*)((char*)base + _offset); - if (fun == NULL) { - return 0; - } - return fun(self, visit, arg); -} - PyObject * PyType_FromSpecWithBases(PyType_Spec *spec, PyObject *bases) { @@ -2985,7 +2924,7 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases) } } - res = (PyHeapTypeObject*)PyType_FromSpec_Alloc(&PyType_Type, nmembers); + res = (PyHeapTypeObject*)PyType_GenericAlloc(&PyType_Type, nmembers); if (res == NULL) return NULL; res_start = (char*)res; @@ -3093,30 +3032,6 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases) memcpy(PyHeapType_GET_MEMBERS(res), slot->pfunc, len); type->tp_members = PyHeapType_GET_MEMBERS(res); } - else if (slot->slot == Py_tp_traverse) { - - /* Types created by PyType_FromSpec own a strong reference to their - * type, but this was added in Python 3.8. The tp_traverse function - * needs to call Py_VISIT on the type but all existing traverse - * functions cannot be updated (especially the ones from existing user - * functions) so we need to provide a tp_traverse that manually calls - * Py_VISIT(Py_TYPE(self)) and then call the provided tp_traverse. In - * this way, user functions do not need to be updated, preserve - * backwards compatibility. - * - * We store the user-provided traverse function at the end of the type - * (we have allocated space for it) so we can call it from our - * PyType_FromSpec_tp_traverse wrapper. - * - * Check bpo-40217 for more information and rationale about this issue. - * - * */ - - type->tp_traverse = PyType_FromSpec_tp_traverse; - size_t _offset = _PyObject_VAR_SIZE(&PyType_Type, nmembers+1); - traverseproc *user_traverse = (traverseproc*)((char*)type + _offset); - *user_traverse = slot->pfunc; - } else { /* Copy other slots directly */ *(void**)(res_start + slotoffsets[slot->slot]) = slot->pfunc; diff --git a/Parser/asdl_c.py b/Parser/asdl_c.py index f8729cd170b..ce9724aee3e 100755 --- a/Parser/asdl_c.py +++ b/Parser/asdl_c.py @@ -673,6 +673,7 @@ ast_dealloc(AST_object *self) static int ast_traverse(AST_object *self, visitproc visit, void *arg) { + Py_VISIT(Py_TYPE(self)); Py_VISIT(self->dict); return 0; } diff --git a/Python/Python-ast.c b/Python/Python-ast.c index d2edf74c812..694987dd077 100644 --- a/Python/Python-ast.c +++ b/Python/Python-ast.c @@ -1109,6 +1109,7 @@ ast_dealloc(AST_object *self) static int ast_traverse(AST_object *self, visitproc visit, void *arg) { + Py_VISIT(Py_TYPE(self)); Py_VISIT(self->dict); return 0; }