classification
Title: issubclass(obj, abc.ABC) causes a segfault
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: inada.naoki, izbyshev, jab, levkivskyi, miss-islington, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2018-03-05 16:19 by izbyshev, last changed 2018-03-23 09:43 by miss-islington. This issue is now closed.

Files
File name Uploaded Description Edit
stack-trace.txt izbyshev, 2018-03-05 16:19
Pull Requests
URL Status Linked Edit
PR 6002 merged inada.naoki, 2018-03-06 11:31
PR 6014 merged miss-islington, 2018-03-07 07:28
PR 5944 jab, 2018-03-07 07:43
PR 6189 closed inada.naoki, 2018-03-22 11:59
PR 6190 merged miss-islington, 2018-03-22 12:53
PR 6197 merged inada.naoki, 2018-03-23 09:04
PR 6198 merged miss-islington, 2018-03-23 09:20
Messages (18)
msg313259 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2018-03-05 16:19
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
msg313261 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2018-03-05 16:28
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.
msg313266 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-05 16:48
Is there any sense in accepting non-types as the first argument of issubclass()? I would add a check just in issubclass().
msg313269 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2018-03-05 17:01
> 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.
msg313284 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2018-03-05 20:46
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.
msg313321 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2018-03-06 11:57
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.
msg313322 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2018-03-06 12:12
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).
msg313333 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-06 15:28
Could you please show examples with __suclasscheck__ returning True for non-class objects?
msg313369 - (view) Author: Inada Naoki (inada.naoki) * (Python committer) Date: 2018-03-07 00:57
> 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.
msg313374 - (view) Author: Inada Naoki (inada.naoki) * (Python committer) Date: 2018-03-07 07:27
New changeset fc7df0e664198cb05cafd972f190a18ca422989c by INADA Naoki in branch 'master':
bpo-32999: Fix ABC.__subclasscheck__ crash (GH-6002)
https://github.com/python/cpython/commit/fc7df0e664198cb05cafd972f190a18ca422989c
msg313375 - (view) Author: miss-islington (miss-islington) Date: 2018-03-07 07:47
New changeset d824b4e4afbe9cf02310e39b14402fb2aa271f8f by Miss Islington (bot) in branch '3.7':
bpo-32999: Fix ABC.__subclasscheck__ crash (GH-6002)
https://github.com/python/cpython/commit/d824b4e4afbe9cf02310e39b14402fb2aa271f8f
msg313377 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2018-03-07 08:06
@inada.naoki: I don't question your change. My point is the same as in #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 #33018 might make sense.
msg313378 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2018-03-07 08:07
Sorry for status update, this was due to a concurrent modification.
msg313396 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2018-03-07 19:22
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).
msg314251 - (view) Author: Inada Naoki (inada.naoki) * (Python committer) Date: 2018-03-22 12:52
New changeset f757b72b2524ce3451d2269f0b8a9f0593a7b27f by INADA Naoki in branch 'master':
bpo-32999: Revert GH-6002 (fc7df0e6) (GH-6189)
https://github.com/python/cpython/commit/f757b72b2524ce3451d2269f0b8a9f0593a7b27f
msg314256 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2018-03-22 14:00
New changeset 5d8bb5d07be2a9205e7059090f0ac5360d36b217 by Ivan Levkivskyi (Miss Islington (bot)) in branch '3.7':
bpo-32999: Revert GH-6002 (fc7df0e6) (GH-6189) (GH-6190)
https://github.com/python/cpython/commit/5d8bb5d07be2a9205e7059090f0ac5360d36b217
msg314300 - (view) Author: Inada Naoki (inada.naoki) * (Python committer) Date: 2018-03-23 09:19
New changeset c65bf3fe4a2bde424b79e350f36b7aaa3f6476f6 by INADA Naoki in branch 'master':
bpo-32999: ast: Convert useless check to assert (GH-6197)
https://github.com/python/cpython/commit/c65bf3fe4a2bde424b79e350f36b7aaa3f6476f6
msg314301 - (view) Author: miss-islington (miss-islington) Date: 2018-03-23 09:43
New changeset c71edab15d023360388da8360700d419b5f48c81 by Miss Islington (bot) in branch '3.7':
bpo-32999: ast: Convert useless check to assert (GH-6197)
https://github.com/python/cpython/commit/c71edab15d023360388da8360700d419b5f48c81
History
Date User Action Args
2018-03-23 09:43:17miss-islingtonsetmessages: + msg314301
2018-03-23 09:20:00miss-islingtonsetpull_requests: + pull_request5945
2018-03-23 09:19:37inada.naokisetmessages: + msg314300
2018-03-23 09:04:26inada.naokisetpull_requests: + pull_request5944
2018-03-22 14:00:14levkivskyisetmessages: + msg314256
2018-03-22 12:53:03miss-islingtonsetpull_requests: + pull_request5935
2018-03-22 12:52:45inada.naokisetmessages: + msg314251
2018-03-22 11:59:00inada.naokisetpull_requests: + pull_request5933
2018-03-07 19:22:21levkivskyisetmessages: + msg313396
2018-03-07 08:07:11izbyshevsetstatus: open -> closed

nosy: + jab
messages: + msg313378

resolution: fixed
2018-03-07 08:06:13izbyshevsetstatus: closed -> open

nosy: - jab
messages: + msg313377

resolution: fixed -> (no value)
2018-03-07 07:53:10jabsetnosy: + jab
2018-03-07 07:52:32inada.naokisetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-03-07 07:47:42miss-islingtonsetnosy: + miss-islington
messages: + msg313375
2018-03-07 07:43:09jabsetpull_requests: + pull_request5780
2018-03-07 07:28:11miss-islingtonsetpull_requests: + pull_request5779
2018-03-07 07:27:06inada.naokisetmessages: + msg313374
2018-03-07 00:57:06inada.naokisetmessages: + msg313369
2018-03-06 15:28:25serhiy.storchakasetmessages: + msg313333
2018-03-06 12:12:45izbyshevsetmessages: + msg313322
2018-03-06 11:57:13levkivskyisetmessages: + msg313321
2018-03-06 11:31:47inada.naokisetkeywords: + patch
stage: patch review
pull_requests: + pull_request5767
2018-03-06 06:55:39izbyshevsetversions: + Python 3.7
2018-03-05 20:46:38izbyshevsetmessages: + msg313284
2018-03-05 17:01:47izbyshevsetmessages: + msg313269
versions: - Python 3.7
2018-03-05 16:48:55serhiy.storchakasetnosy: + inada.naoki, serhiy.storchaka

messages: + msg313266
versions: + Python 3.7
2018-03-05 16:28:19izbyshevsetmessages: + msg313261
2018-03-05 16:19:41izbyshevcreate