_PyType_GetModuleByDef optimization is incorrect

[encukou]

BPO	46433
Nosy	@vstinner, @encukou, @miss-islington, @erlend-aasland
PRs	bpo-46433: _PyType_GetModuleByDef: handle static types in MRO #30696 [3.10] bpo-46433: _PyType_GetModuleByDef: handle static types in MRO (GH-30696) #31262

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2022-02-11.11:26:15.883>
created_at = <Date 2022-01-19.15:46:37.594>
labels = ['3.10', '3.11']
title = '_PyType_GetModuleByDef optimization is incorrect'
updated_at = <Date 2022-02-11.12:23:43.696>
user = 'https://github.com/encukou'

bugs.python.org fields:

activity = <Date 2022-02-11.12:23:43.696>
actor = 'erlendaasland'
assignee = 'none'
closed = True
closed_date = <Date 2022-02-11.11:26:15.883>
closer = 'petr.viktorin'
components = []
creation = <Date 2022-01-19.15:46:37.594>
creator = 'petr.viktorin'
dependencies = []
files = []
hgrepos = []
issue_num = 46433
keywords = ['patch']
message_count = 11.0
messages = ['410962', '411166', '411167', '412370', '412482', '412498', '412500', '413012', '413058', '413063', '413064']
nosy_count = 4.0
nosy_names = ['vstinner', 'petr.viktorin', 'miss-islington', 'erlendaasland']
pr_nums = ['30696', '31262']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue46433'
versions = ['Python 3.10', 'Python 3.11']

[encukou]

I'm looking at the _PyType_GetModuleByDef optimization in https://github.com/python/cpython/pull/25504/files -- previously I assumed it's OK since it passed review.
But it doesn't look correct:

• in the _PyType_HasFeature assert, we should be looking at super rather than type
• it's definitely possible that a hear type has a static type in the MRO -- object, at least. The (PyHeapTypeObject*)super cast is invalid in the case when the module is not found. And it's also incorrect in some cases of diamond inheritance, when a static type comes before the type we're looking for in bases.

It also adds a precondition that's not feasible public API, which this was meant to become: it should be callable with any type object. That's possible to do by keeping faster internal API and adding a public version with checks, but the diamond inheritance problem remains. Py_TPFLAGS_HEAPTYPE needs to be checked at runtime (except for the first iteration, if we're sure we're handling a static type).

Does that analysis sound right?

[vstinner]

Oh, it seems like I misunderstood type_ready_mro() change. The check is only done on static types type. The MRO of heap types is not checked.

--

In my change, I wrote:

    // _PyType_GetModuleByDef() must only be called on a heap type created
    // by PyType_FromModuleAndSpec() or on its subclasses.
    // type_ready_mro() ensures that a static type cannot inherit from a
    // heap type.

based on this function:

static int
type_ready_mro(PyTypeObject *type)
{
    /* Calculate method resolution order */
    if (mro_internal(type, NULL) < 0) {
        return -1;
    }
    assert(type->tp_mro != NULL);
    assert(PyTuple_Check(type->tp_mro));

    /* All bases of statically allocated type should be statically allocated */
    if (!(type->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
        PyObject *mro = type->tp_mro;
        Py_ssize_t n = PyTuple_GET_SIZE(mro);
        for (Py_ssize_t i = 0; i < n; i++) {
            PyTypeObject *base = (PyTypeObject *)PyTuple_GET_ITEM(mro, i);
            if (PyType_Check(base) && (base->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
                PyErr_Format(PyExc_TypeError,
                             "type '%.100s' is not dynamically allocated but "
                             "its base type '%.100s' is dynamically allocated",
                             type->tp_name, base->tp_name);
                return -1;
            }
        }
    }
    return 0;
}

This code comes from bpo-22079:

commit e09bcc8
Author: Serhiy Storchaka <storchaka@gmail.com>
Date: Wed Jan 28 11:03:33 2015 +0200

Issue bpo-22079: PyType_Ready() now checks that statically allocated type has
no dynamically allocated bases.

Rationale: https://bugs.python.org/issue22079#msg236830

--

Note: PyType_Ready() function was very big and complex. I splitted this huge function into sub-functions in bpo-43770. I hope that it's a little bit more readable yet. I tried but failed to find a bug about tp_getattro and tp_setattro inheritance

[vstinner]

An alternative would be to merge PyHeapTypeObject members into PyTypeObject. I don't get why there is a separated struture: PyTypeObject is excluded from the stable ABI. See my remark about that:
https://bugs.python.org/issue22079#msg391464

Having a separated structure just static types make the code more complex and more error prone.

[miss-islington]

New changeset 0ef0853 by Petr Viktorin in branch 'main':
bpo-46433: _PyType_GetModuleByDef: handle static types in MRO (GH-30696)
0ef0853

[erlend-aasland]

It looks like this can be closed. Petr?

[encukou]

Almost. It's a bugfix so it needs backports to 3.10 & 3.9.
Thanks for the reminder!
I should get to them next week.

[erlend-aasland]

Right, this of course affects 3.10 and 3.9. Let me know if you want me to do the backports :)

(Updating version field for this ticket)

[encukou]

Just 3.10, after all. 3.9 doesn't have the function yet.

I did the backport, but I'd welcome a review by a fresh set of eyes!