Repository navigation
Flawed assumptions about tp_dictoffset in inheritance. #95589
Description
Activity
Here's the weakref version:
from _testcapi import HeapCTypeWithWeakref class I3(HeapCTypeWithWeakref, list): pass i = I3() i.append(0) i.weakreflist print("OK")$ python3.10 test.py Segmentation fault (core dumped) $ python3.12 test.py Segmentation fault (core dumped)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.+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.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.
Revert the inheritance behavior from 3.10
Can you do that today/tomorrow, for the RC?
As only one base class can have opaque data, there would be only be one valid tp_visit_data, which speeds things up.
Only one direct base, but its base can also have opaque data. Still it'd be only one chain to walk for data&slots.
Can you do that today/tomorrow, for the RC?
Sure.
@Fidget-Spinner already has #95242 open, but I'll need to add tests.Reacted by Petr ViktorinOnly one direct base, but its base can also have opaque data.
That's the responsibility of the direct base, since both must be written in C to do that.
both must be written in C to do that.
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 ofsuper()correctly (as shown, in part, bysubtype_visitwith managed dicts).Can you do that today/tomorrow, for the RC?
Sure. @Fidget-Spinner already has #95242 open, but I'll need to add tests.
Superseded by #95596.
True, but they might be from different libraries with different release schedules.
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.
- added a commit that references this issue
on Aug 17, 2022
In Python, the
__dict__and__weakref__slots are treated specially (slots meaning__slots__, nottp_slots)They are automatically insert by the VM when creating a class.
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 thetp_dictoffsetandtp_weaklistoffsetoffsets.But, if either field is accessed directly, then we access invalid memory and 💥
test.py:
$ 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