Skip to content
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

issubclass(obj, abc.ABC) causes a segfault #77180

Closed
izbyshev mannequin opened this issue Mar 5, 2018 · 18 comments
Closed

issubclass(obj, abc.ABC) causes a segfault #77180

izbyshev mannequin opened this issue Mar 5, 2018 · 18 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@izbyshev
Copy link
Mannequin

izbyshev mannequin commented Mar 5, 2018

BPO 32999
Nosy @jab, @methane, @serhiy-storchaka, @ilevkivskyi, @izbyshev, @miss-islington
PRs
  • bpo-32999: Fix ABC.__subclasscheck__ crash #6002
  • [3.7] bpo-32999: Fix ABC.__subclasscheck__ crash (GH-6002) #6014
  • bpo-33018: Improve issubclass() error checking and message. #5944
  • bpo-32999: Revert GH-6002 (fc7df0e6) #6189
  • [3.7] bpo-32999: Revert GH-6002 (fc7df0e6) (GH-6189) #6190
  • bpo-32999: Convert useless check to assert #6197
  • [3.7] bpo-32999: ast: Convert useless check to assert (GH-6197) #6198
  • Files
  • stack-trace.txt
  • 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 = None
    closed_at = <Date 2018-03-07.08:07:11.825>
    created_at = <Date 2018-03-05.16:19:41.316>
    labels = ['extension-modules', '3.7', '3.8', 'type-crash']
    title = 'issubclass(obj, abc.ABC) causes a segfault'
    updated_at = <Date 2018-03-23.09:43:17.020>
    user = 'https://github.com/izbyshev'

    bugs.python.org fields:

    activity = <Date 2018-03-23.09:43:17.020>
    actor = 'miss-islington'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-03-07.08:07:11.825>
    closer = 'izbyshev'
    components = ['Extension Modules']
    creation = <Date 2018-03-05.16:19:41.316>
    creator = 'izbyshev'
    dependencies = []
    files = ['47470']
    hgrepos = []
    issue_num = 32999
    keywords = ['patch']
    message_count = 18.0
    messages = ['313259', '313261', '313266', '313269', '313284', '313321', '313322', '313333', '313369', '313374', '313375', '313377', '313378', '313396', '314251', '314256', '314300', '314301']
    nosy_count = 6.0
    nosy_names = ['jab', 'methane', 'serhiy.storchaka', 'levkivskyi', 'izbyshev', 'miss-islington']
    pr_nums = ['6002', '6014', '5944', '6189', '6190', '6197', '6198']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue32999'
    versions = ['Python 3.7', 'Python 3.8']

    @izbyshev
    Copy link
    Mannequin Author

    izbyshev mannequin commented Mar 5, 2018

    Demo:

    >>> from abc import ABC
    >>> issubclass(1, ABC)
    Segmentation fault (core dumped)

    The stack trace is attached.

    Before reimplementation of abc in C, the result was confusing too:

    Python 3.6.4 (v3.6.4:d48eceb, Dec 19 2017, 06:54:40) [MSC v.1900 64 bit (AMD64)]
     on win32
    >>> from abc import ABC
    >>> issubclass(1, ABC)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "abc.py", line 230, in __subclasscheck__
      File "_weakrefset.py", line 84, in add
    TypeError: cannot create weak reference to 'int' object

    @izbyshev izbyshev mannequin added 3.8 only security fixes extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump labels Mar 5, 2018
    @izbyshev
    Copy link
    Mannequin Author

    izbyshev mannequin commented Mar 5, 2018

    In debug mode, the following assertion fails:

    python: ./Modules/_abc.c:642: _abc__abc_subclasscheck_impl: Assertion `((((((PyObject*)(mro))->ob_type))->tp_flags & ((1UL << 26))) != 0)' failed.

    @serhiy-storchaka
    Copy link
    Member

    Is there any sense in accepting non-types as the first argument of issubclass()? I would add a check just in issubclass().

    @serhiy-storchaka serhiy-storchaka added the 3.7 (EOL) end of life label Mar 5, 2018
    @izbyshev
    Copy link
    Mannequin Author

    izbyshev mannequin commented Mar 5, 2018

    Is there any sense in accepting non-types as the first argument of issubclass()?

    No, though it is not (clearly) documented. The docs mention TypeError, but only for the second argument if my reading is correct.

    In practice, issubclass() raises a TypeError if the first argument is not a class object:

    >>> issubclass(1, int)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: issubclass() arg 1 must be a class

    Though, as I mentioned above, behavior for ABCs was always weird.

    @izbyshev izbyshev mannequin removed the 3.7 (EOL) end of life label Mar 5, 2018
    @izbyshev
    Copy link
    Mannequin Author

    izbyshev mannequin commented Mar 5, 2018

    I've also checked that ABC.register() doesn't allow non-classes (and PEP-3119 mentions that).

    Looking at PyObject_IsSubclass in Objects/abstract.c, the only case in which its check_class() could be avoided is if there is a custom __subclasscheck__:

    >>> class M(type):
    ...   def __subclasscheck__(cls, c):
    ...     return c == 1 or super().__subclasscheck__(c)
    ...
    >>> class A(metaclass=M):
    ...   pass
    ...
    >>> issubclass(1, A)
    True

    If there is no need to support such weird __subclasscheck__, check_class() could be called earlier.

    Note, however, that check_class() treats anything having __bases__ as a class, so moving the check alone is not enough to avoid the crash in all cases.

    @izbyshev izbyshev mannequin added the 3.7 (EOL) end of life label Mar 6, 2018
    @ilevkivskyi
    Copy link
    Member

    Actually, the behaviour when __suclasscheck__ returns True for non-class objects may be used by some code. Even typing module did this, I tried to remove as much as possible of this, but I think there may be few such situations left.

    Therefore, the patch by Inada-san (that makes C behaviour follow Python behaviour) looks reasonable to me.

    @izbyshev
    Copy link
    Mannequin Author

    izbyshev mannequin commented Mar 6, 2018

    OK, making a new implementation behave as the old one is fine with me too.

    BTW, do TypeErrors related to weak references deserve any treatment? Isn't it a kind of coincidence that the error raised due to usage of WeakSet in issubclass(obj, ABC) is what we expect? (Sorry, I'm not familiar with WeakSets).

    @serhiy-storchaka
    Copy link
    Member

    Could you please show examples with __suclasscheck__ returning True for non-class objects?

    @methane
    Copy link
    Member

    methane commented Mar 7, 2018

    BTW, do TypeErrors related to weak references deserve any treatment? Isn't it a kind of coincidence that the error raised due to usage of WeakSet in issubclass(obj, ABC) is what we expect? (Sorry, I'm not familiar with WeakSets).

    Sorry, I can't get what is your point.
    I don't want to change ABC behavior for now. I want to make C implementation consistent with Python implementation, except some (unrealistic) corner cases.

    @methane
    Copy link
    Member

    methane commented Mar 7, 2018

    New changeset fc7df0e by INADA Naoki in branch 'master':
    bpo-32999: Fix ABC.__subclasscheck__ crash (GH-6002)
    fc7df0e

    @miss-islington
    Copy link
    Contributor

    New changeset d824b4e by Miss Islington (bot) in branch '3.7':
    bpo-32999: Fix ABC.__subclasscheck__ crash (GH-6002)
    d824b4e

    @methane methane closed this as completed Mar 7, 2018
    @izbyshev
    Copy link
    Mannequin Author

    izbyshev mannequin commented Mar 7, 2018

    @inada.naoki: I don't question your change. My point is the same as in bpo-33018 (I've discovered that PR only after I commented). The error message is misleading, and it's just a coincidence that a TypeError and not some other error is raised when abc attempts to add a non-type object to a WeakSet.

    @serhiy.storchaka: Note that an example that you requested is unlikely to be related to ABC and probably is more like my artificial __subclasscheck__ example. So, making changes just for ABC as suggested in bpo-33018 might make sense.

    @izbyshev izbyshev mannequin reopened this Mar 7, 2018
    @izbyshev
    Copy link
    Mannequin Author

    izbyshev mannequin commented Mar 7, 2018

    Sorry for status update, this was due to a concurrent modification.

    @izbyshev izbyshev mannequin closed this as completed Mar 7, 2018
    @ilevkivskyi
    Copy link
    Member

    Serhiy, for example issubclass(typing.MutableMapping, typing.Mapping) returns True while neither of those two are actual class objects. These relationships are kept mostly so that typing.* can be used as a drop-in replacement for collections.abc.* (plus backwards compatibility).

    @methane
    Copy link
    Member

    methane commented Mar 22, 2018

    New changeset f757b72 by INADA Naoki in branch 'master':
    bpo-32999: Revert #50252 (fc7df0e) (GH-6189)
    f757b72

    @ilevkivskyi
    Copy link
    Member

    New changeset 5d8bb5d by Ivan Levkivskyi (Miss Islington (bot)) in branch '3.7':
    bpo-32999: Revert #50252 (fc7df0e) (GH-6189) (GH-6190)
    5d8bb5d

    @methane
    Copy link
    Member

    methane commented Mar 23, 2018

    New changeset c65bf3f by INADA Naoki in branch 'master':
    bpo-32999: ast: Convert useless check to assert (GH-6197)
    c65bf3f

    @miss-islington
    Copy link
    Contributor

    New changeset c71edab by Miss Islington (bot) in branch '3.7':
    bpo-32999: ast: Convert useless check to assert (GH-6197)
    c71edab

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants