On 09.12.19 23:26, Nick Coghlan wrote: > > > On Tue., 10 Dec. 2019, 5:17 am MRAB, <pyt...@mrabarnett.plus.com > <mailto:pyt...@mrabarnett.plus.com>> wrote: > > On 2019-12-09 18:22, Christian Tismer wrote: > > > > > > Hi Nick, > > > > after staring long at the code, I fount something funny in > > typeobject.c #286 ff: > > > > > > static void > > type_mro_modified(PyTypeObject *type, PyObject *bases) { > > /* > > Check that all base classes or elements of the MRO of type are > > able to be cached. This function is called after the base > > classes or mro of the type are altered. > > > > Unset HAVE_VERSION_TAG and VALID_VERSION_TAG if the type > > has a custom MRO that includes a type which is not officially > > super type, or if the type implements its own mro() method. > > > > Called from mro_internal, which will subsequently be called on > > each subclass when their mro is recursively updated. > > */ > > Py_ssize_t i, n; > > int custom = (Py_TYPE(type) != &PyType_Type); > > int unbound; > > PyObject *mro_meth = NULL; > > PyObject *type_mro_meth = NULL; > > > > if (!PyType_HasFeature(type, Py_TPFLAGS_HAVE_VERSION_TAG)) > > return; > > > > if (custom) { > > _Py_IDENTIFIER(mro); > > mro_meth = lookup_maybe_method( > > (PyObject *)type, &PyId_mro, &unbound); > > if (mro_meth == NULL) > > goto clear; > > type_mro_meth = lookup_maybe_method( > > (PyObject *)&PyType_Type, &PyId_mro, &unbound); > > if (type_mro_meth == NULL) > > goto clear; > > if (mro_meth != type_mro_meth) > > goto clear; > > Py_XDECREF(mro_meth); > > Py_XDECREF(type_mro_meth); > > } > > > > > > Look at the "if (custom)" clause. > > "mro_meth = lookup_maybe_method(" uses lookup_maybe_method which > > gives a borrowed reference. The same holds for "type_mro_meth". > > > > But then both are decreffed, which IMHO is not correct. > > > Look at what happens at the label "clear": it DECREFs them. > > If mro_meth != NULL or mro_meth != type_mro_meth, they'll get DECREFed > at "clear". > > > I believe Christian's point is that this entire "if (custom) {" branch > looks suspicious, as it assumes "lookup_maybe_method" will increment the > refcount on the returned object. If that assumption is incorrect, we're > going to get DECREFs without a preceding INCREF. > > The specific code path is also obscure enough that it's plausible the > test suite may not currently cover it (as it requires doing something > that calls "type_mro_modified" on a type with a custom metaclass).
Thanks Nick for this nice analysis. And it's exactly this codepath that is taken in the case of PySide: custom types all the time :-) what-a-relief - ly y'rs -- Chris -- Christian Tismer-Sperling :^) tis...@stackless.com Software Consulting : http://www.stackless.com/ Karl-Liebknecht-Str. 121 : https://www.qt.io/qt-for-python 14482 Potsdam : GPG key -> 0xE7301150FB7BEE0E phone +49 173 24 18 776 fax +49 (30) 700143-0023
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-le...@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/5MTJC47BHGZW6RKKU5JDAIFLV5P6W6JA/ Code of Conduct: http://python.org/psf/codeofconduct/
