issubclass(obj, abc.ABC) causes a segfault

[izbyshev]

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]

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]

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]

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

[izbyshev]

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]

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.

[ilevkivskyi]

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]

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]

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

[methane]

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.