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.

Sign up for GitHub

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

Jump to bottom

bpo-34441: Fix ABC.__subclasscheck__ crash on a class with invalid __subclasses__ #8835

Merged
merged 9 commits into from
Aug 20, 2018
Merged

bpo-34441: Fix ABC.__subclasscheck__ crash on a class with invalid __subclasses__ #8835

merged 9 commits into from
Aug 20, 2018

Conversation

izbyshev
Copy link
Contributor

@izbyshev izbyshev commented Aug 20, 2018 •
edited
Loading

The missing NULL check was reported by Svace static analyzer.

https://bugs.python.org/issue34441

bpo-34441: Fix ABC.__subclasscheck__ crash on a class with non-callab…
377d01b 
…le __subclasses__

The missing NULL check was reported by Svace static analyzer.
ilevkivskyi
ilevkivskyi approved these changes Aug 20, 2018
View reviewed changes
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

serhiy-storchaka
serhiy-storchaka reviewed Aug 20, 2018
View reviewed changes
Lib/test/test_abc.py Outdated
@@ -410,6 +410,13 @@ class C:
with self.assertRaises(TypeError):
issubclass(C(), A)

# bpo-34441: Check that issubclass() doesn't crash on bogus classes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add also tests for following cases:

  • __subclasses__ is a callable with wrong signature.
  • __subclasses__ raises an exception explicitly.
  • __subclasses__ returns not a list.
  • __subclasses__ returns a list containing not a type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've added those cases.

berkerpeksag
berkerpeksag reviewed Aug 20, 2018
View reviewed changes
Lib/test/test_abc.py Outdated
lambda: [42],
]

for bs in bogus_subclasses:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: bs -> cls

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it would be nice to use a subTest() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: bs -> cls

Why? bs denotes (would-be) callables, not classes.

And it would be nice to use a subTest() here.

Thank you, fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then may be use func?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK!

Lib/test/test_abc.py Outdated
issubclass(int, S)

# Also check that issubclass() propagates exceptions raised by
# __subclasses__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Please end all sentences with a full stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst Outdated
@@ -0,0 +1,2 @@
Fix crash when an ``ABC``-derived class with non-callable ``__subclasses__`` is passed as the
second argument to ``issubclass()``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

``issubclass()`` -> :func:`issubclass`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add "Patch by Your Name." and wrap lines at 80 chars.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thank you.

serhiy-storchaka
serhiy-storchaka reviewed Aug 20, 2018
View reviewed changes
Lib/test/test_abc.py Outdated
# bpo-34441: Check that issubclass() doesn't crash on bogus classes
bogus_subclasses = [
None,
lambda x: None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it returning a valid result: [].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

serhiy-storchaka
serhiy-storchaka reviewed Aug 20, 2018
View reviewed changes
Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst Outdated
@@ -1,2 +1,3 @@
Fix crash when an ``ABC``-derived class with non-callable ``__subclasses__`` is passed as the
second argument to ``issubclass()``.
Fix crash when an ``ABC``-derived class with non-callable ``__subclasses__``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-callable __subclasses__ is not the only cause of a crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I've also updated the PR name.

@izbyshev izbyshev changed the title bpo-34441: Fix ABC.__subclasscheck__ crash on a class with non-callab… bpo-34441: Fix ABC.__subclasscheck__ crash on a class with invalid __subclasses__ Aug 20, 2018
berkerpeksag
berkerpeksag reviewed Aug 20, 2018
View reviewed changes
Lib/test/test_abc.py Outdated
__subclasses__ = func

with self.subTest(i=i), \
self.assertRaises(TypeError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly a cosmetic change, I find the following style more readable (and we usually try to avoid using a backslash if it's possible to write a readable code):

with self.subTest(i=i):
    self.assertRaises(TypeError, issubclass, int, S)

Or:

with self.subTest(i=i):
    with self.assertRaises(TypeError):
        issubclass(int, S)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I went for the second one for consistency with other checks.

berkerpeksag
berkerpeksag approved these changes Aug 20, 2018
View reviewed changes
Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you! :)

@berkerpeksag berkerpeksag merged commit cdbf50c into python:master Aug 20, 2018
@miss-islington
Copy link
Contributor

Thanks @izbyshev for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 20, 2018
@miss-islington
bpo-34441: Fix ABC.__subclasscheck__ crash on classes with invalid __…
bd41aeb 
…subclasses__ (pythonGH-8835)

The missing NULL check was reported by Svace static analyzer.
(cherry picked from commit cdbf50c)

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
@bedevere-bot
Copy link

GH-8840 is a backport of this pull request to the 3.7 branch.

@izbyshev izbyshev deleted the bpo-34441 branch August 20, 2018 20:30
miss-islington added a commit that referenced this pull request Aug 20, 2018
@miss-islington
bpo-34441: Fix ABC.__subclasscheck__ crash on classes with invalid __…
d1f0ccc 
…subclasses__ (GH-8835)

The missing NULL check was reported by Svace static analyzer.
(cherry picked from commit cdbf50c)

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@ilevkivskyi ilevkivskyi ilevkivskyi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@izbyshev @miss-islington @bedevere-bot @berkerpeksag @serhiy-storchaka @ilevkivskyi @the-knights-who-say-ni