New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The danger of PyType_FromSpec() #71166
Comments
There is a catch when convert static types to dynamic types using PyType_FromSpec(). tp_new is not inherited for static types whose base class is 'object' (see a comment in inherit_special() in Objects/typeobject.c:4569 for explanation). Such types left not callable and not pickleable unless tp_new is explicitly specified. But since PyType_FromSpec() creates heap types, tp_new is inherited from the base 'object' class. Converted types unexpectedly becomes callable and pickleable. Since object.__new__ wouldn't insure the invariants that the extension type's own factory function ensures, instantiated object can be in inconsistent state. Using it can lead to a crash. We already fallen two time in this catch with _tkinter (bpo-15721) and curses.panel (bpo-14936) modules (fixed in bpo-23815). We should document this catch and a way to avoid it (explicitly set tp_new to NULL for the result of PyType_FromSpec()). Both the C API documentation and PEP-384 should be updated. Would be great if standard examples include a case of non-callable heap type. As option we can introduce more handy way to set tp_new to NULL by adding {Py_tp_new, NULL} to slots array (currently this doesn't have any effect). But this is new feature. |
There is another catch that belongs in the same category. There is line 2841 in typeobject.c if (type->tp_dealloc == NULL) {
/* It's a heap type, so needs the heap types' dealloc.
subtype_dealloc will call the base type's tp_dealloc, if
necessary. */
type->tp_dealloc = subtype_dealloc;
} When converting the PySide types, it became a real problem. Types with an And another source of incompatibility: PyType_FromSpec() enforces a tp_name field to be dotted, to compute a module A small but remarkable glitch: |
I don't think PEP-384 should be updated. It documents the feature as it was added, and shouldn't be used as documentation now. For this issue, the fix should be updating C API documentation and, for Pyhon 3.8+, making {*, NULL} slots override the inherited value. I think the PyType_Spec/PyType_Slot copying should have its own issue. |
Christian, do you have a specific example of the default tp_dealloc doing the wrong thing? |
Hi Petr, yes I have that what generated the wrong thing, but it is inside All types which had been no heaptypes before were already quite complicated When switching to the new interface, those deallocators which were 0 were By replacing it by a dummy function fixed the problem. Example: static PyType_Slot PySideSignalMetaType_slots[] = { You can find the checkins here. This branch contains everything relevant in small steps. The first working transformation was in the commit I have yet no idea how to extract a minimum example that produces this problem. Summary: (sorry, there is more...) |
Thanks! I think that explains enough of the issue. Converting static types to heap ones is just one way you can use PyType_Spec. Another is writing new types, which should work much like Python classes. So I don't think we should change the default, but rather document the use case. I'd rather update the docs after #53400 is merged, so this is blocked now. I'll also try making an explicit |
Petr, Fine! PySide's problems are solved for now, no worries. Problem 1) There is another thing that gives problems: When creating types which have fields "behind" the normal type fields, We changed that into a normal PyType declaration (opaque) and a pointer For that, we needed sizeof(PyType_Type). Ok, that could be solved by Python Problem 2) Harder is this: For some operations, we needed access to fields of normal types, for instance the PyType_GetSlot forbids access to normal types, rigorously. This uses a property of types which has not explicitly been used:
That means, we can allow access to all "tp_" fields with simply changing a check I'm not sure if that information should be made official. May be it should be hidden For PySide, it was crucial to use that information to compute the offset of Please let me know if I can help with something! Cheers - Chris |
|
No. To obtain sizeof(PyType_Type), we simply used the Python code
It is a simple trick, but probably very few people know this path
That's my question. Do you want to hide the fact that some fields I would simply give access to tp_* for all types and that's it.
Yes. I need access to PyType_Type's tp_new field. PyType_Type is a very central structure in Python, and it really hurts You can see the use of this field here:
Not being able to use this field led to many weird workaround attempts, We could maybe generate a heaptype clone of PyType_Type and grab the function Maybe there also is a way to avoid the use of this function all together. Maybe we approach it again. At that time, it was not even clear that we |
Ah! It seems you don't need access to all tp_* slots of any type here, but just to PyType.tp_new – only one specific type, and only one specific slot. Is that right? Or was this only an example of a larger problem? (Sorry for drawing the discussion out! Dropping the checks in PyType_GetSlot would be a simple change technically, but it doesn't match my mental model of what the stable ABI should be. I'm hoping to get a better solution for your use case, and others like it.)
Well, basicsize might be exposed through Python, but it's still not part of the limited API. Which is fine – all that means is you might need to change the extension for some future version of Python. I'm sure your tests will tell you when the time comes, and I hope we'll have a better solution then! |
Yes, you are exactly right. I needed access to very few fields.
The latter would not be necessary for PEP-384, but for my __signature__ <basicsize>
Oh? I thought it is automatically allowed to use if it is exposed Sure, we could have worked without the size and create a more complex
Yes, I had a very hard time to convince myself that I _may_ use some
Thank you! I was hoping to get ready after 2 months, when I realized that Ciao -- Chris |
If somebody is interested to see how the transition to the https://github.com/pyside/pyside2-setup/blob/5.11/sources/shiboken2/libshiboken/pep384impl_doc.rst |
The default of PyType_FromSpec for tp_dealloc is wrong! After a long struggle with crashes and leaks, the situation When a type is converted from a static type to a heaptype This is not correct. In the given type, the only change is to become a heaptype. Before this solution, I used a dummy function to prevend It was hard to understand that the default for a static type The now correctly working workaround is to explicitly insert Again, in order to use this fix, it is necessary to break the |
I close the issue.
Issued fixed by adding Py_TPFLAGS_DISALLOW_INSTANTIATION to Python 3.10.
Since Python 3.8, the dealloc function must now decrement a reference to its type: https://docs.python.org/dev/c-api/typeobj.html#c.PyTypeObject.tp_dealloc Having a NULL tp_dealloc may be treated as an error. @ctismer: Your feedback is very valuable. Please open issues for remaining issues which are not fixed in Python 3.13. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: