On 09.12.19 23:26, Nick Coghlan wrote:
On Tue., 10 Dec. 2019, 5:17 am MRAB, <python@mrabarnett.plus.com <mailto:python@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 :^) tismer@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