Skip to content

Ensure in PyType_Ready() that base class of static type is static #66277

New issue
New issue
Open
Open
Ensure in PyType_Ready() that base class of static type is static#66277
Assignees
serhiy-storchaka
Labels
interpreter-core(Objects, Python, Grammar, and Parser dirs)topic-C-APItype-featureA feature request or enhancement

Description

@serhiy-storchaka
serhiy-storchaka
opened on Jul 26, 2014
BPO 22079
Nosy @doko42, @pitrou, @scoder, @vstinner, @alex, @encukou, @serhiy-storchaka, @jdemeyer, @zhangyangyu
Files
  • issue22079.patch
  • issue22079-tp_bases.patch
    		•

    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:

    assignee = 'https://github.com/serhiy-storchaka'
closed_at = None
created_at = <Date 2014-07-26.12:36:39.011>
labels = ['interpreter-core', 'type-feature']
title = 'Ensure in PyType_Ready() that base class of static type is static'
updated_at = <Date 2022-01-24.12:23:03.373>
user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2022-01-24.12:23:03.373>
actor = 'petr.viktorin'
assignee = 'serhiy.storchaka'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2014-07-26.12:36:39.011>
creator = 'serhiy.storchaka'
dependencies = []
files = ['37211', '43541']
hgrepos = []
issue_num = 22079
keywords = ['patch', 'needs review']
message_count = 26.0
messages = ['224049', '224102', '224103', '224110', '231277', '231579', '234852', '234876', '236807', '236830', '238881', '269188', '269195', '269223', '269247', '269275', '269278', '269279', '269291', '269339', '326321', '368736', '368743', '391446', '391464', '411469']
nosy_count = 11.0
nosy_names = ['doko', 'pitrou', 'scoder', 'vstinner', 'Arfrever', 'alex', 'petr.viktorin', 'python-dev', 'serhiy.storchaka', 'jdemeyer', 'xiang.zhang']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22079'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    Activity

    1. serhiy-storchaka commented on Jul 26, 2014

      @serhiy-storchaka
      serhiy-storchaka
      on Jul 26, 2014
      MemberAuthor

      It would be good if PyType_Ready() will check that base class of static type is static.

    2. serhiy-storchaka
      added
      interpreter-core(Objects, Python, Grammar, and Parser dirs)
      type-featureA feature request or enhancement
      on Jul 26, 2014

    3. pitrou commented on Jul 27, 2014

      @pitrou
      pitrou
      on Jul 27, 2014
      Member

      By static, you mean not a heap type?

    4. alex commented on Jul 27, 2014

      @alex
      alex
      on Jul 27, 2014
      Member

      Yup.

    5. serhiy-storchaka commented on Jul 27, 2014

      @serhiy-storchaka
      serhiy-storchaka
      on Jul 27, 2014
      MemberAuthor

      By static, you mean not a heap type?

      Yes. Sorry for bad wording.

    6. serhiy-storchaka commented on Nov 17, 2014

      @serhiy-storchaka
      serhiy-storchaka
      on Nov 17, 2014
      MemberAuthor

      Here is a patch.

    7. serhiy-storchaka commented on Nov 23, 2014

      @serhiy-storchaka
      serhiy-storchaka
      on Nov 23, 2014
      MemberAuthor

      Could anyone please make a review?

    8. serhiy-storchaka commented on Jan 27, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Jan 27, 2015
      MemberAuthor

      Ping.

    9. python-dev commented on Jan 28, 2015

      @python-dev
      python-devmannequin
      on Jan 28, 2015
      Mannequin

      New changeset c087ac6fc171 by Serhiy Storchaka in branch '2.7':
      Issue bpo-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 bpo-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 bpo-22079: PyType_Ready() now checks that statically allocated type has
      https://hg.python.org/cpython/rev/eb26255e11f1

    10. serhiy-storchaka
      closed this as completedon Feb 5, 2015
    11. serhiy-storchaka
      self-assigned this
      on Feb 5, 2015

    12. doko42 commented on Feb 27, 2015

      @doko42
      doko42
      on Feb 27, 2015
      Member

      reopening, this breaks some stuff in several places ...

      https://bugs.launchpad.net/ubuntu/+source/terminator/+bug/1426294

    13. doko42
      reopened this on Feb 27, 2015

    14. serhiy-storchaka commented on Feb 27, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Feb 27, 2015
      MemberAuthor

      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.

    15. 5 remaining items

    16. serhiy-storchaka commented on Jun 25, 2016

      @serhiy-storchaka
      serhiy-storchaka
      on Jun 25, 2016
      MemberAuthor

      __bases__ != mro()

    17. zhangyangyu commented on Jun 26, 2016

      @zhangyangyu
      zhangyangyu
      on Jun 26, 2016
      Member

      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

    18. serhiy-storchaka commented on Jun 26, 2016

      @serhiy-storchaka
      serhiy-storchaka
      on Jun 26, 2016
      MemberAuthor

      Good point! tp_bases should be iterated instead of tp_mro in this check.

    19. serhiy-storchaka
      reopened this on Jun 26, 2016

    20. zhangyangyu commented on Jun 26, 2016

      @zhangyangyu
      zhangyangyu
      on Jun 26, 2016
      Member

      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?

    21. serhiy-storchaka commented on Jun 26, 2016

      @serhiy-storchaka
      serhiy-storchaka
      on Jun 26, 2016
      MemberAuthor

      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.

    22. zhangyangyu commented on Jun 27, 2016

      @zhangyangyu
      zhangyangyu
      on Jun 27, 2016
      Member

      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.

    23. jdemeyer commented on Sep 25, 2018

      @jdemeyer
      jdemeyer
      on Sep 25, 2018
      Contributor

      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.

    24. pitrou commented on May 12, 2020

      @pitrou
      pitrou
      on May 12, 2020
      Member

      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)

    25. scoder commented on May 12, 2020

      @scoder
      scoder
      on May 12, 2020
      Contributor

      Since it's not clear from this ticket what the original problem was, is there a chance it could have been related to bpo-35810?

    26. scoder commented on Apr 20, 2021

      @scoder
      scoder
      on Apr 20, 2021
      Contributor

      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 bpo-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.

    27. vstinner commented on Apr 20, 2021

      @vstinner
      vstinner
      on Apr 20, 2021
      Member

      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.

    28. encukou commented on Jan 24, 2022

      @encukou
      encukou
      on Jan 24, 2022
      Member

      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.

    29. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    30. erlend-aasland
      added
      topic-C-API
      on Jan 17, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    Labels

    interpreter-core(Objects, Python, Grammar, and Parser dirs)topic-C-APItype-featureA feature request or enhancement

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions