Setting negative tp_dictoffset on PyLong subclass leads to crash

[encukou]

Bug report

Bug description:

In Python 3.12, it is no longer possible to subclass int by extending basicsize and setting a negative tp_dictoffset.

The 3.11 docs suggest that this might work:

A negative offset is more expensive to use, and should only be used when the instance structure contains a variable-length part. This is used for example to add an instance variable dictionary to subtypes of str or tuple.

That is, while int is not given as an explicit example, but there's nothing to imply it would be different than str or tuple.

Indeed, it works on Python 3.11, but fails in 3.12 when accessing the __dict__, as Python calls _PyObject_ComputedDictPointer, which tries to compute the dict offset using abs(ob_size), which is invalid for int on 3.12.

I'm not the best person to judge whether this is a bad bug, acceptable backwards-incompatible change, or perhaps it's a misuse of 3.11 API (but I can't see exactly how it's misused).

The 3.12 What's new does mention tp_dictoffset-related breaking changes:

The use of tp_dictoffset and tp_weaklistoffset is still supported, but does not fully support multiple inheritance (gh-95589), and performance may be worse.

But multiple inheritance is not in play here.

@markshannon, any thoughts?

---

Reproducer (hope I didn't simplify too much):

extmodule.c:

#include "Python.h"
#include "structmember.h"   // for PyMemberDef API in 3.11 & below

static PyTypeObject *basetype = &PyLong_Type;

static int
mod_exec(PyObject *m)
{
    PyObject *type = PyType_FromModuleAndSpec(
        m,
        &(PyType_Spec) {
            .name = "ext.MySubclass",
            .basicsize = basetype->tp_basicsize + sizeof(PyDictObject*),
            .flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
            .slots = (PyType_Slot[]) {
                {Py_tp_members, (PyMemberDef[]) {
                    {
                        "__dictoffset__", T_PYSSIZET,
                        -sizeof(PyDictObject*), READONLY},
                    {NULL},
                }},
                {0, 0}, 
            }},
        (PyObject*)basetype);
    if (!type) return -1;
    if (PyModule_AddType(m, (PyTypeObject*)type) == -1) return -1;
    Py_DECREF(type);

    return 0;
}

static struct PyModuleDef mod_def = {
    PyModuleDef_HEAD_INIT,
    .m_name = "ext",
    .m_slots = (PyModuleDef_Slot[]) {
        {Py_mod_exec, mod_exec},
        {0, NULL}
    },
};

PyMODINIT_FUNC
PyInit_ext(void)
{
    return PyModuleDef_Init(&mod_def);
}

repro.py:

import ext
print(ext)

obj = ext.MySubclass(1)
obj.attr = 'foo'
assert obj.attr == 'foo'

Compiling is tricky with distutils gone, but on Fedora a command to compile & run is:
gcc $(python3.12-config --cflags --ldflags) -fPIC --shared extmodule.c -o ext.so && python3.12d -Xdev repro.py

Crashes on 3.12, works on 3.11 or with basetype = &PyUnicode_Type.

CPython versions tested on:

3.11, 3.12

Operating systems tested on:

Linux

[markshannon]

Has anyone reported this as an actual issue, or is it just a hypothetical problem?
Fixing this will be awkward, and unless it is a real problem fixing it feels like wasted effort compared to getting new, more robust, APIs for class creation in place.
Maybe we should just recommend using Py_TPFLAGS_MANAGED_DICT as that's what you get if you subclass int in Python.

>>> Py_TPFLAGS_MANAGED_DICT = 1 << 4
>>> class I(int): pass
>>> I.__flags__ & Py_TPFLAGS_MANAGED_DICT 
16

[markshannon]

We could treat any negative tp_dictoffset offset as equivalent to setting Py_TPFLAGS_MANAGED_DICT.

Since you're not supposed to do you own dictionary offset computations, but rely on _PyObject_GetDictPtr, it doesn't matter if we place the dictionary before the header instead of after the array.
It should work as long as the object is allocated using the C-API not using malloc or something like that.

[encukou]

Has anyone reported this as an actual issue, or is it just a hypothetical problem?

Yes, Thrameos from jpype.

Fixing this will be awkward [...] maybe we should just recommend using Py_TPFLAGS_MANAGED_DICT

How would that work? Document it in 3.12 What's New?

you're not supposed to do you own dictionary offset computations

Or was this the breaking change, between 3.10 and 3.11?

[markshannon]

How would that work? Document it in 3.12 What's New?

It's documented here https://docs.python.org/3/c-api/typeobj.html#c.Py_TPFLAGS_MANAGED_DICT (the 3.12 docs might need updating)

Or was this the breaking change, between 3.10 and 3.11?

Yes.

My guess is that no one did that calculation when accessing the __dict__ so we didn't see any issues at the time, but some code did do it when initializing instances.
It is possible that some instances of extension classes ended up with two dictionary slots: the VM allocated one which was used as the __dict__, and an extension allocated one that was unused.

[markshannon]

@encukou You say in the issue that "nothing to imply it would be different than str or tuple."
Is this an issue for str or tuple? I suspect this is an int only issue, in which case the fix is quite simple.

[encukou]

It'd be an issue for any subclassable type whose abs(Py_SIZE) doesn't give the actual size. I guess int is the only one that changed this way? (You're probably the one who'd know that.)

If so, I guess the way is to:

1. special-case _PyObject_ComputedDictPointer to use _PyLong_DigitCount for int (and backport that)
2. document that backwards compatibility was broken in 3.11, and what the implications are (and backport that)

I can do it if it sound good to you.