On Tue., 10 Dec. 2019, 5:17 am MRAB, <[email protected]
<mailto:[email protected]>> 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).