bpo-34441: Fix ABC.__subclasscheck__ crash on a class with invalid __subclasses__ by izbyshev · Pull Request #8835 · python/cpython

izbyshev · 2018-08-20T14:05:19Z

The missing NULL check was reported by Svace static analyzer.

https://bugs.python.org/issue34441

…le __subclasses__ The missing NULL check was reported by Svace static analyzer.

ilevkivskyi

Thanks! LGTM

serhiy-storchaka · 2018-08-20T14:52:54Z

Lib/test/test_abc.py

            with self.assertRaises(TypeError):
                issubclass(C(), A)

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


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.

Thanks, I've added those cases.

berkerpeksag · 2018-08-20T16:29:59Z

Lib/test/test_abc.py

+                lambda: [42],
+            ]
+
+            for bs in bogus_subclasses:


Nit: bs -> cls

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

Nit: bs -> cls

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

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

Thank you, fixed.

Then may be use func?

berkerpeksag · 2018-08-20T16:30:59Z

Lib/test/test_abc.py

+                    issubclass(int, S)
+
+            # Also check that issubclass() propagates exceptions raised by
+            # __subclasses__


Nit: Please end all sentences with a full stop.

berkerpeksag · 2018-08-20T16:31:43Z

Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst

@@ -0,0 +1,2 @@
+Fix crash when an ``ABC``-derived class with non-callable ``__subclasses__`` is passed as the
+second argument to ``issubclass()``.


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

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

Fixed, thank you.

serhiy-storchaka · 2018-08-20T17:00:40Z

Lib/test/test_abc.py

+            # bpo-34441: Check that issubclass() doesn't crash on bogus classes
+            bogus_subclasses = [
+                None,
+                lambda x: None,


Make it returning a valid result: [].

serhiy-storchaka · 2018-08-20T17:29:51Z

Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst

@@ -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__``


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

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

berkerpeksag · 2018-08-20T17:49:10Z

Lib/test/test_abc.py

+                    __subclasses__ = func
+
+                with self.subTest(i=i), \
+                        self.assertRaises(TypeError):


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)

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

berkerpeksag

LGTM, thank you! :)

miss-islington · 2018-08-20T20:04:22Z

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

…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 · 2018-08-20T20:04:40Z

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

…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>

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.

the-knights-who-say-ni added the CLA signed label Aug 20, 2018

bedevere-bot added the awaiting review label Aug 20, 2018

ilevkivskyi added the needs backport to 3.7 label Aug 20, 2018

ilevkivskyi approved these changes Aug 20, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Aug 20, 2018

serhiy-storchaka reviewed Aug 20, 2018

View reviewed changes

Add tests for various unusual __subclasses__

fa3bdae

berkerpeksag reviewed Aug 20, 2018

View reviewed changes

Fix NEWS entry and reformat it with blurb

a3cd050

serhiy-storchaka reviewed Aug 20, 2018

View reviewed changes

Alexey Izbyshev added 3 commits August 20, 2018 20:22

End sentences with a full stop

432f6df

Test an invalid __subclasses__ signature with a valid return type

56b21e8

Use subTest() to distinguish iterations

7df825e

serhiy-storchaka reviewed Aug 20, 2018

View reviewed changes

Alexey Izbyshev added 2 commits August 20, 2018 20:35

Generalize NEWS entry

dcf2e04

bs -> func

7d3c5ec

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 reviewed Aug 20, 2018

View reviewed changes

Use nested with statements for readability

72cb031

berkerpeksag approved these changes Aug 20, 2018

View reviewed changes

serhiy-storchaka approved these changes Aug 20, 2018

View reviewed changes

berkerpeksag merged commit cdbf50c into python:master Aug 20, 2018

bedevere-bot removed the awaiting merge label Aug 20, 2018

bedevere-bot removed the needs backport to 3.7 label Aug 20, 2018

izbyshev deleted the bpo-34441 branch August 20, 2018 20:30

		@@ -0,0 +1,2 @@
		Fix crash when an ``ABC``-derived class with non-callable ``__subclasses__`` is passed as the
		second argument to ``issubclass()``.

Uh oh!

Conversation

izbyshev commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Aug 20, 2018

Uh oh!

bedevere-bot commented Aug 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

izbyshev commented Aug 20, 2018 •

edited

Loading