This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Ensure in PyType_Ready() that base class of static type is static
Type: enhancement Stage: patch review
Components: Interpreter Core Versions: Python 3.4, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, alex, doko, jdemeyer, petr.viktorin, pitrou, python-dev, scoder, serhiy.storchaka, vstinner, xiang.zhang
Priority: normal Keywords: needs review, patch

Created on 2014-07-26 12:36 by serhiy.storchaka, last changed 2022-04-11 14:58 by admin.

Files
File name Uploaded Description Edit
issue22079.patch serhiy.storchaka, 2014-11-17 10:42 review
issue22079-tp_bases.patch serhiy.storchaka, 2016-06-26 06:28 review
Messages (26)
msg224049 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-07-26 12:36
It would be good if PyType_Ready() will check that base class of static type is static.
msg224102 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-07-27 04:13
By static, you mean not a heap type?
msg224103 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-07-27 04:15
Yup.
msg224110 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-07-27 07:57
> By static, you mean not a heap type?

Yes. Sorry for bad wording.
msg231277 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-17 10:42
Here is a patch.
msg231579 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-23 21:32
Could anyone please make a review?
msg234852 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-01-27 20:59
Ping.
msg234876 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-01-28 09:10
New changeset c087ac6fc171 by Serhiy Storchaka in branch '2.7':
Issue #22079: PyType_Ready() now checks that statically allocated type has
https://hg.python.org/cpython/rev/c087ac6fc171

New changeset 747855f29b9d by Serhiy Storchaka in branch '3.4':
Issue #22079: PyType_Ready() now checks that statically allocated type has
https://hg.python.org/cpython/rev/747855f29b9d

New changeset eb26255e11f1 by Serhiy Storchaka in branch 'default':
Issue #22079: PyType_Ready() now checks that statically allocated type has
https://hg.python.org/cpython/rev/eb26255e11f1
msg236807 - (view) Author: Matthias Klose (doko) * (Python committer) Date: 2015-02-27 17:59
reopening, this breaks some stuff in several places ...

https://bugs.launchpad.net/ubuntu/+source/terminator/+bug/1426294
msg236830 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-27 20:23
This restriction was added because bad thing could happen when statically allocated class inherits dynamically allocated class. I don't remember the number of the issue where such example was exposed, may be Alex remember it.

Likely pygobject creates classes in not safe manner. It would be more correct to create pygobject classes with PyType_FromSpec(), but current code worked for it for years. May be it is not so easy to change pygobject. So may be we should revert changes in 2.7 or convert the error to the deprecation warning.

Is there similar issue with 3.4? There may be difference due to the difference of the lifetime of types in 2.x and 3.x.
msg238881 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-03-22 07:48
New changeset aa79a04e9bf5 by Serhiy Storchaka in branch '2.7':
Issue #22079: Py3k warning now is issued in  PyType_Ready() instead of
https://hg.python.org/cpython/rev/aa79a04e9bf5

New changeset 57a457ea84e4 by Serhiy Storchaka in branch '3.4':
Issue #22079: Deprecation warning now is issued in  PyType_Ready() instead of
https://hg.python.org/cpython/rev/57a457ea84e4
msg269188 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-06-24 15:33
I think we can start from index 1 instead of 0.
msg269195 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-24 17:15
Can you please explain?
msg269223 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-06-25 07:09
In the mro list I think the first is always the type itself which has already been checked to be a statically allocated type if we can touch the loop. Starting from 0 checks it twice and it's not a base either.

BTW, I don't receive an email from the tracker. :(
msg269247 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-25 19:21
__bases__ != mro()
msg269275 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-06-26 05:10
Of course, I know it. But doesn't the local variable `bases` refer to mro[1]?

[1] https://hg.python.org/cpython/file/tip/Objects/typeobject.c#l4900
msg269278 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-26 06:28
Good point! tp_bases should be iterated instead of tp_mro in this check.
msg269279 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-06-26 06:40
I thought that too at first. But if we use __bases__, can we still guarantee that 'all bases of statically allocated type should be statically allocated'? How about its grand bases? Is it possible that a statically allocated type inherit a dynamic type but does not go through PyType_Ready and then be inherited?
msg269291 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-26 15:02
This is interesting question. It looks that base classes (as well as mro classes) may not go through PyType_Ready. There is no even a check that mro() returns only ancestor classes and contains the initial class itself.
msg269339 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-06-27 02:21
Oh yes. The mro list can be customized so we can not even guarantee the type appears at index 0 and object at index n-1. It seems we should still keep what it is now, iterating the whole mro list.

Sorry that my previous comments seem to be noise. But maybe we can add one comment to clarify that iterating the whole list is needed.
msg326321 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2018-09-25 05:20
> It would be good if PyType_Ready() will check that base class of static type is static.

What's the rationale for this change? It's not explained in this bug report nor in the code.
msg368736 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2020-05-12 16:29
The workaround that Cython had to added for this (temporarily enable Py_TPFLAGS_HEAPTYPE when calling PyType_Ready()) is fragile. It would be nice to rethink the approach here, or disable the check altogether.

(or perhaps, expose another C API function - such as PyType_ReadyEx() - that allows disabling the check)
msg368743 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2020-05-12 19:56
Since it's not clear from this ticket what the original problem was, is there a chance it could have been related to issue 35810?
msg391446 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2021-04-20 15:55
Coming back to this after a while. I would like to get rid of the work-around (read: huge hack) that we have in Cython for this check and thus would ask for the check to be removed in Py3.10.

According to the discussion, no-one seemed to remember why it was added in the first place, just that "bad things happened", but not what or how.

With the fix for issue 35810 in place since Py3.8, my guess is that the situation that this check is preventing has actually become safe now. Unless someone can motivate that the risks were other than a premature deallocation of the base type.
msg391464 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-04-20 18:37
I recently reworked type_new() and PyType_New() in bpo-43770. I wrote documentation for the type_ready_inherit_as_structs() helper function of PyType_Ready():

// For static types, inherit tp_as_xxx structures from the base class
// if it's NULL.
//
// For heap types, tp_as_xxx structures are not NULL: they are set to the
// PyHeapTypeObject.as_xxx fields by type_new_alloc().
static void
type_ready_inherit_as_structs(PyTypeObject *type, PyTypeObject *base)

I hesitated to add assertions to ensure that fields are set if the type is a heap type.

One issue with static type is that it doesn't have the following fields of PyHeapTypeObject:

    PyAsyncMethods as_async;
    PyNumberMethods as_number;
    PyMappingMethods as_mapping;
    PySequenceMethods as_sequence; /* as_sequence comes after as_mapping,
                                      so that the mapping wins when both
                                      the mapping and the sequence define
                                      a given operator (e.g. __getitem__).
                                      see add_operators() in typeobject.c . */
    PyBufferProcs as_buffer;

Is there a reason to not add these fields to PyTypeObject? PyTypeObject is not part of the stable ABI.

Type inheritance without these fields make PyType_Ready() weird and more complex.
msg411469 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2022-01-24 12:23
> Is there a reason to not add these fields to PyTypeObject?


Can't say I haven't thought of that, but AFAIK it would mean breaking the C API substantially. Even if not it'd be a PEP-sized change, IMO.
History
Date User Action Args
2022-04-11 14:58:06adminsetgithub: 66277
2022-01-24 12:23:03petr.viktorinsetnosy: + petr.viktorin
messages: + msg411469
2021-04-20 18:37:15vstinnersetmessages: + msg391464
2021-04-20 15:55:17scodersetmessages: + msg391446
2020-05-12 19:56:43scodersetnosy: + scoder
messages: + msg368743
2020-05-12 16:29:57pitrousetmessages: + msg368736
2018-09-25 05:20:02jdemeyersetnosy: + jdemeyer
messages: + msg326321
2016-06-27 02:21:28xiang.zhangsetmessages: + msg269339
2016-06-26 15:02:27serhiy.storchakasetmessages: + msg269291
2016-06-26 06:40:23xiang.zhangsetmessages: + msg269279
2016-06-26 06:28:03serhiy.storchakasetstatus: closed -> open
files: + issue22079-tp_bases.patch
messages: + msg269278

resolution: fixed ->
stage: resolved -> patch review
2016-06-26 05:10:57xiang.zhangsetmessages: + msg269275
2016-06-25 19:21:09serhiy.storchakasetmessages: + msg269247
2016-06-25 07:09:13xiang.zhangsetmessages: + msg269223
2016-06-24 17:15:23serhiy.storchakasetmessages: + msg269195
2016-06-24 15:33:17xiang.zhangsetnosy: + xiang.zhang
messages: + msg269188
2015-03-30 07:05:09serhiy.storchakasetstatus: open -> closed
resolution: fixed
2015-03-22 13:02:52Arfreversetnosy: + Arfrever
2015-03-22 07:48:16python-devsetmessages: + msg238881
2015-02-27 20:23:51serhiy.storchakasetmessages: + msg236830
2015-02-27 17:59:08dokosetstatus: closed -> open

nosy: + doko
messages: + msg236807

resolution: fixed -> (no value)
2015-02-05 12:32:57serhiy.storchakasetstatus: open -> closed
assignee: serhiy.storchaka
resolution: fixed
stage: patch review -> resolved
2015-01-28 09:10:31python-devsetnosy: + python-dev
messages: + msg234876
2015-01-27 20:59:00serhiy.storchakasetmessages: + msg234852
2014-11-23 21:32:37serhiy.storchakasetmessages: + msg231579
2014-11-17 10:42:17serhiy.storchakasetkeywords: + needs review, patch
files: + issue22079.patch
messages: + msg231277

stage: needs patch -> patch review
2014-07-27 07:57:13serhiy.storchakasetmessages: + msg224110
2014-07-27 04:15:46alexsetmessages: + msg224103
2014-07-27 04:13:22pitrousetnosy: + pitrou
messages: + msg224102
2014-07-26 12:36:39serhiy.storchakacreate