Repository navigation
Flawed assumptions about tp_dictoffset in inheritance. #95589
Description
In Python, the __dict__ and __weakref__ slots are treated specially (slots meaning __slots__, not tp_slots)
They are automatically insert by the VM when creating a class.
class C(list):
pass
>>> C().__dict__
{}In order to support inheritance, specifically multiple inheritance, the VM can lay out subclasses in ways that differ from the superclass.
This is OK, provided __dict__ and __weakref__ are only accessed though the tp_dictoffset and tp_weaklistoffset offsets.
But, if either field is accessed directly, then we access invalid memory and 💥
test.py:
from _testcapi import HeapCTypeWithDict
class I3(HeapCTypeWithDict, list): pass
i = I3()
i.append(0)
print(i.dictobj)$ python3.10 ~/test/test.py
Segmentation fault (core dumped)We have (accidentally) fixed this for __dict__ in 3.11, although at the expense breaking backwards compatibility for some C extensions. However, the problem still remains for __weakref__.
Backwards incompatibility
from _testcapi import HeapCTypeWithDict
class I3(HeapCTypeWithDict, list): pass
print("OK")$ python3.10 test.py
OK
$ python3.12 test.py
Traceback (most recent call last):
File "test.py", line 3, in <module>
class I3(HeapCTypeWithDict, list): pass
TypeError: multiple bases have instance lay-out conflict
Activity
markshannon commentedon Aug 3, 2022
Here's the weakref version:
markshannon commentedon Aug 3, 2022
Proposed fix
3.11
Revert the inheritance behavior from 3.10
Although this introduces possible crashes, it should help pybind11 and mypyc port to 3.11.
3.12
For 3.12 we should re-introduce the breaking, but not crashing, change and extend it to weakrefs.
We then need to provide a better, and properly documented, API for for C extension classes to reliably
use
__dict__s and weakrefs managed by the VM.encukou commentedon Aug 3, 2022
+1. Though IMO it would be better to treat 3.10 behavior (only
Py_TYPE(obj)->tp_dictoffsetis valid) as the status quo, and the improvements as a change.For the better API, my current thinking is adding API for clearing/visiting only a single type's data, so e.g.
tp_visitwould call each base'stp_visit_datain a loop, with “VM managed” things (dict, weaklist, type) handled only in CPython code. Users could still override all oftp_visitfor backcompat (and/or speed?), but we'd discourage it.markshannon commentedon Aug 3, 2022
As only one base class can have opaque data, there would be only be one valid
tp_visit_data, which speeds things up.There are only about 10 or so valid permutations of pre-headers, opaque data, and slots, so we can generate a complete set of efficient visit, clear and dealloc functions for use in subclasses.
encukou commentedon Aug 3, 2022
Can you do that today/tomorrow, for the RC?
Only one direct base, but its base can also have opaque data. Still it'd be only one chain to walk for data&slots.
markshannon commentedon Aug 3, 2022
Sure.
@Fidget-Spinner already has #95242 open, but I'll need to add tests.
markshannon commentedon Aug 3, 2022
That's the responsibility of the direct base, since both must be written in C to do that.
encukou commentedon Aug 3, 2022
True, but they might be from different libraries with different release schedules. The other library might be CPython, with its changing requirements on what needs to be visited – or it could be pybind11 or numpy or whatever, CPython shouldn't need special treatment here for its pre-headers.
An alternative would be that you're expected to delegate to the base, but it's surprisingly tricky to do the equivalent of
super()correctly (as shown, in part, bysubtype_visitwith managed dicts).Fidget-Spinner commentedon Aug 3, 2022
Superseded by #95596.
markshannon commentedon Aug 5, 2022
That's not possible. If there were then the only way they could be combined is in Python, which would raise a TypeError due to the incompatible layout.
GH-95589: Dont crash when subclassing extension classes with multiple…