[issue26979] The danger of PyType_FromSpec()
New submission from Serhiy Storchaka: 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 (issue15721) and curses.panel (issue14936) modules (fixed in issue23815). 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. ---------- assignee: docs@python components: Documentation messages: 265152 nosy: docs@python, loewis, serhiy.storchaka priority: high severity: normal status: open title: The danger of PyType_FromSpec() type: security versions: Python 3.5, Python 3.6 _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26979> _______________________________________
Change by Petr Viktorin <encukou@gmail.com>: ---------- nosy: +Dormouse759, encukou _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Change by Nick Coghlan <ncoghlan@gmail.com>: ---------- nosy: +ncoghlan _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Change by Terry J. Reedy <tjreedy@udel.edu>: ---------- stage: -> needs patch versions: +Python 3.7, Python 3.8 -Python 3.5 _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Christian Tismer <tismer@stackless.com> added the comment: 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 existing tp_dealloc behaved normal, but those with tp_dealloc=NULL gave segfaults. I ended up supplying a default dummy_dealloc() that is just there to occupy this slot. Then it worked. And another source of incompatibility: PyType_FromSpec() enforces a tp_name field to be dotted, to compute a module name. Very annoying incompatibility that could be avoided. A small but remarkable glitch: Most fields of the PyType_Spec/PyType_Slot combo are copied, but some are not. So expect funny behaviour when creating types with dynamically generated specs without strdup. That should IMHO be changed. ---------- nosy: +Christian.Tismer _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Petr Viktorin <encukou@gmail.com> added the comment: 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. Instead, Python documentation should be improved to cover all of PEP 389. I submitted a pull request for that (GH-9154). 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. ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Petr Viktorin <encukou@gmail.com> added the comment: Christian, do you have a specific example of the default tp_dealloc doing the wrong thing? ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Christian Tismer <tismer@stackless.com> added the comment: Hi Petr, yes I have that what generated the wrong thing, but it is inside a specific PySide repository in a big project. Before I try to extract and simulate that, let me just show it. All types which had been no heaptypes before were already quite complicated types which had their own deallocators before, or a 0 if they did not need one, or better "should not have one". When switching to the new interface, those deallocators which were 0 were turned into the default deallocator, which crashed most of the time. By replacing it by a dummy function fixed the problem. Example: static PyType_Slot PySideSignalMetaType_slots[] = { {Py_tp_methods, (void *)Signal_methods}, {Py_tp_base, (void *)&PyType_Type}, {Py_tp_free, (void *)PyObject_GC_Del}, {Py_tp_dealloc, (void *)SbkDummyDealloc}, {0, 0} }; static PyType_Spec PySideSignalMetaType_spec = { "PySide2.QtCore.MetaSignal", 0, // sizeof(PyHeapTypeObject) is filled in by PyType_FromSpecWithBases // which calls PyType_Ready which calls inherit_special. 0, Py_TPFLAGS_DEFAULT, PySideSignalMetaType_slots, }; You can find the checkins here. This branch contains everything relevant in small steps. https://github.com/pyside/pyside2-setup/commit/2f0658be36107097810985f2190fe... The first working transformation was in the commit "PEP 384-8-HT-4: Successful Restart using PyType_FromSpec". I have yet no idea how to extract a minimum example that produces this problem. Summary: In a way, without being heaptypes, the old types were dynamic, too, and inserting something that was not there before was a bad idea. (sorry, there is more...) ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Petr Viktorin <encukou@gmail.com> added the comment: 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 GH-9154 is merged, so this is blocked now. I'll also try making an explicit `{Py_tp_dealloc, NULL}` override the inherited value, as per Serhiy's suggestion. But it'll only go to 3.8, so it won't help PySide now. ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Christian Tismer <tismer@stackless.com> added the comment: Petr, Fine! PySide's problems are solved for now, no worries. But as mentioned, there is more. Problem 1) ---------- There is another thing that gives problems: When creating types which have fields "behind" the normal type fields, PySide had explicit structs that contained PyHeaptypeObject explicitly, plus the extra fields. We changed that into a normal PyType declaration (opaque) and a pointer expression that dynamically computed the offset. For that, we needed sizeof(PyType_Type). Ok, that could be solved by Python code, but this "trick" is not so obvious and should be documented, or a variable should be provided that gives the heap type size, dynamically. Problem 2) ---------- Harder is this: For some operations, we needed access to fields of normal types, for instance the tp_new field of PyType_Type. That is not possible, although it would be easy: PyType_GetSlot forbids access to normal types, rigorously. But concerning the "normal" type fields, those which are not indirections, it would cost nothing to allow this for normal types. This uses a property of types which has not explicitly been used: * All types have the same basic structure * That means, we can allow access to all "tp_" fields with simply changing a check in PyType_GetSlot. I'm not sure if that information should be made official. May be it should be hidden and PyType_GetSlot should be made more complicated. But the proposed change comes at no cost. And the prefixes like "nb_" or "sq_" are still visible, so I don't think there was an intent to make type structures completely opaque? For PySide, it was crucial to use that information to compute the offset of certain fields of normal types, which is a bit of a Pep 384 breach. We wrote code that verifies that these assumptions are all valid, but I would be happy to remove that and revert to only using PyType_GetSlot. Please let me know if I can help with something! Cheers - Chris ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Petr Viktorin <encukou@gmail.com> added the comment: 1) If I understand correctly, this problem could be solved by per-class C storage? Something like PyModuleDef.m_size / PyModule_GetState, but for classes? 2) Making PyType_GetSlot work for static types would not be trivial. All types do have the same *basic* structure (the tp_* fields), but the nb_*/sq_*/etc. fields don't need to be in the same order, and might not exist at all. Even making it work for the tp_* fields only would bake in some assumptions I do want to get rid of. I'd like to understand your use case better. Do you have a specific example here? ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Christian Tismer <tismer@stackless.com> added the comment:
1) If I understand correctly, this problem could be solved by per-class C storage? Something like PyModuleDef.m_size / PyModule_GetState, but for classes?
No. To obtain sizeof(PyType_Type), we simply used the Python code type.__basicsize__ It is a simple trick, but probably very few people know this path to get that info. At least, for me it took way too long ;-)
2) Making PyType_GetSlot work for static types would not be trivial. All types do have the same *basic* structure (the tp_* fields), but the nb_*/sq_*/etc. fields don't need to be in the same order, and might not exist at all. Even making it work for the tp_* fields only would bake in some assumptions I do want to get rid of.
That's my question. Do you want to hide the fact that some fields are direct and others are indirect? I don't think that will ever be changed before Python 4.0, so why put this burden on every user, then? I would simply give access to tp_* for all types and that's it.
I'd like to understand your use case better. Do you have a specific example here?
Yes. I need access to PyType_Type's tp_new field. PyType_Type is a very central structure in Python, and it really hurts to hide every attribute away. You can see the use of this field here: https://github.com/pyside/pyside2-setup/blob/5.11/sources/shiboken2/libshibo... // The meta type creates a new type when the Python programmer extends a wrapped C++ class. newfunc type_new = reinterpret_cast<newfunc>(PyType_Type.tp_new); SbkObjectType *newType = reinterpret_cast<SbkObjectType*>(type_new(metatype, args, kwds)); if (!newType) return 0; Not being able to use this field led to many weird workaround attempts, which did not really work. We tried to call this function from Python code, but then there are several checks which make it impossible to use. We could maybe generate a heaptype clone of PyType_Type and grab the function from there. But this is quite obscure and cannot be the recommended way to get at the tp_new function? Maybe there also is a way to avoid the use of this function all together. But we did not want to re-implement a central function of a huge project that we only understood good enough to change it a bit. Maybe we approach it again. At that time, it was not even clear that we would succeed with the limited API. Now that we know, we can try more. ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Change by Neil Schemenauer <nas-python@arctrix.com>: ---------- nosy: +nascheme _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Petr Viktorin <encukou@gmail.com> added the comment: 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. Specifically, you need a way to create class with a metaclass, from C. 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.)
To obtain sizeof(PyType_Type), we simply used the Python code type.__basicsize__ It is a simple trick, but probably very few people know this path to get that info. At least, for me it took way too long ;-)
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! BTW, I'm honestly very impressed how far PySide got with the limited API! ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Christian Tismer <tismer@stackless.com> added the comment:
... Specifically, you need a way to create class with a metaclass, from C.
Is that right? Or was this only an example of a larger problem?
Yes, you are exactly right. I needed access to very few fields. In particular: PyTypeObject.tp_init PyTypeObject.tp_basicsize (workaround using Python) <a_few_types>.tp_dict The latter would not be necessary for Pep 384, but for my __signature__ extension, but that is another story :) <basicsize>
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.
Oh? I thought it is automatically allowed to use if it is exposed by Python. I thought that the central property of basicsize would be that it is no longer a constant, but that a size is always there. Sure, we could have worked without the size and create a more complex structure that does not know anything about types. I actually started with that approach. But in the end I thought it would not hurt to assume that there is a (variable) size, as long as we are below Python 4.0 .
I'm sure your tests will tell you when the time comes, and I hope we'll have a better solution then!
Yes, I had a very hard time to convince myself that I _may_ use some assumptions, as long as I always prove that these assumptions are right. But in a way, I feel guilty and would prefer to go without any trickery.
BTW, I'm honestly very impressed how far PySide got with the limited API!
Thank you! I was hoping to get ready after 2 months, when I realized that all types needed to be changed. So I pulled teeth, and it took >5 months. Btw., I don't understand how the PyQt5 guy(s) solved this. Maybe his structure is way cleaner and simpler than the PySide mess? I'll ask him. Ciao -- Chris ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Christian Tismer <tismer@stackless.com> added the comment: If somebody is interested to see how the transition to the Limited API was done, here is the documentation that I was forced to write :) https://github.com/pyside/pyside2-setup/blob/5.11/sources/shiboken2/libshibo... ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
Christian Tismer <tismer@stackless.com> added the comment: The default of PyType_FromSpec for tp_dealloc is wrong! ------------------------------------------------------- After a long struggle with crashes and leaks, the situation was finally clarified: When a type is converted from a static type to a heaptype via PyType_FromSpec, and the tp_dealloc field is NULL, then subtype_dealloc is inserted. This is not correct. The reasoning in the code is that a heaptype has subtype_dealloc. But before the conversion, the type was static, and for static types the function object_dealloc was inserted. In the given type, the only change is to become a heaptype. The by default inserted object_dealloc should not be changed, since the existing code was written with object_dealloc in mind and not the consequences of replacing it with subtype_dealloc. Before this solution, I used a dummy function to prevend subpype_dealloc being inserted, but that caused leaks. It was hard to understand that the default for a static type is object_dealloc. After that, it was easy to fix that: The now correctly working workaround is to explicitly insert an object_dealloc function whenever the default for tp_dealloc is NULL. Again, in order to use this fix, it is necessary to break the Limited API, because in order to write an object_dealloc function (it is not public) you need access to type objects. ---------- _______________________________________ Python tracker <report@bugs.python.org> <https://bugs.python.org/issue26979> _______________________________________
participants (6)
-
Christian Tismer -
Neil Schemenauer
-
Nick Coghlan
-
Petr Viktorin
-
Serhiy Storchaka
-
Terry J. Reedy