I'm quite interested in the rest of the story here. PySide is probably
the biggest open-source user of the limited API, so IMO it is relevant
to this list.
On 2019-12-11 23:48, Christian Tismer wrote:
Hi all,
Sorry for the noise, I was wrong, and I retract.
I was somehow mislead and hunted a phantom.
Does that mean that there was never a problem?
Or that a problem is still there, but we don't know if it's in CPython
or PySide or their interaction?
Or was the problem solved in PySide?
Do we need to document something better, or just aware of some caveat?
Best - Chris
On 10.12.19 00:29, Christian Tismer wrote:
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
_______________________________________________
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/AONFRH53HED5RN7VVT375JDMORGKEFQV/
Code of Conduct: http://python.org/psf/codeofconduct/
