From 377d01be39ae46f13fe436539b0a116ef03a1af9 Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Mon, 20 Aug 2018 16:41:46 +0300 Subject: [PATCH 1/9] bpo-34441: Fix ABC.__subclasscheck__ crash on a class with non-callable __subclasses__ The missing NULL check was reported by Svace static analyzer. --- Lib/test/test_abc.py | 7 +++++++ .../next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst | 2 ++ Modules/_abc.c | 3 +++ 3 files changed, 12 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst diff --git a/Lib/test/test_abc.py b/Lib/test/test_abc.py index 6fc3c95e4a645e..bc67efb8ecb74e 100644 --- a/Lib/test/test_abc.py +++ b/Lib/test/test_abc.py @@ -410,6 +410,13 @@ class C: with self.assertRaises(TypeError): issubclass(C(), A) + # bpo-34441: Check that issubclass() doesn't crash on bogus classes + class S(metaclass=abc_ABCMeta): + __subclasses__ = None + + with self.assertRaises(TypeError): + issubclass(int, S) + def test_all_new_methods_are_called(self): class A(metaclass=abc_ABCMeta): pass diff --git a/Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst b/Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst new file mode 100644 index 00000000000000..9fe619818d6a5e --- /dev/null +++ b/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()``. diff --git a/Modules/_abc.c b/Modules/_abc.c index 562a2e6d730dd3..ce9140fd03ccc4 100644 --- a/Modules/_abc.c +++ b/Modules/_abc.c @@ -665,6 +665,9 @@ _abc__abc_subclasscheck_impl(PyObject *module, PyObject *self, /* 6. Check if it's a subclass of a subclass (recursive). */ subclasses = PyObject_CallMethod(self, "__subclasses__", NULL); + if (subclasses == NULL) { + goto end; + } if (!PyList_Check(subclasses)) { PyErr_SetString(PyExc_TypeError, "__subclasses__() must return a list"); goto end; From fa3bdaece1367c0bfb6839a4a4aa49d67c84392f Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Mon, 20 Aug 2018 18:21:26 +0300 Subject: [PATCH 2/9] Add tests for various unusual __subclasses__ --- Lib/test/test_abc.py | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_abc.py b/Lib/test/test_abc.py index bc67efb8ecb74e..5064f5ba2f2496 100644 --- a/Lib/test/test_abc.py +++ b/Lib/test/test_abc.py @@ -411,10 +411,31 @@ class C: issubclass(C(), A) # bpo-34441: Check that issubclass() doesn't crash on bogus classes + bogus_subclasses = [ + None, + lambda x: None, + lambda: 42, + lambda: [42], + ] + + for bs in bogus_subclasses: + class S(metaclass=abc_ABCMeta): + __subclasses__ = bs + + with self.assertRaises(TypeError): + issubclass(int, S) + + # Also check that issubclass() propagates exceptions raised by + # __subclasses__ + exc_msg = "exception from __subclasses__" + + def raise_exc(): + raise Exception(exc_msg) + class S(metaclass=abc_ABCMeta): - __subclasses__ = None + __subclasses__ = raise_exc - with self.assertRaises(TypeError): + with self.assertRaisesRegex(Exception, exc_msg): issubclass(int, S) def test_all_new_methods_are_called(self): From a3cd05068d1f3f4559c8e62ed42a88db4f1d55ce Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Mon, 20 Aug 2018 20:03:09 +0300 Subject: [PATCH 3/9] Fix NEWS entry and reformat it with blurb --- .../next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst b/Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst index 9fe619818d6a5e..1a74cfca46e626 100644 --- a/Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst +++ b/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__`` +is passed as the second argument to :func:`issubclass()`. Patch by Alexey +Izbyshev. From 432f6df196f6aa6dda1ee00747fc58f4fd94b5cf Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Mon, 20 Aug 2018 20:20:05 +0300 Subject: [PATCH 4/9] End sentences with a full stop --- Lib/test/test_abc.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_abc.py b/Lib/test/test_abc.py index 5064f5ba2f2496..fc34207a01443e 100644 --- a/Lib/test/test_abc.py +++ b/Lib/test/test_abc.py @@ -410,7 +410,8 @@ class C: with self.assertRaises(TypeError): issubclass(C(), A) - # bpo-34441: Check that issubclass() doesn't crash on bogus classes + # bpo-34441: Check that issubclass() doesn't crash on bogus + # classes. bogus_subclasses = [ None, lambda x: None, @@ -426,7 +427,7 @@ class S(metaclass=abc_ABCMeta): issubclass(int, S) # Also check that issubclass() propagates exceptions raised by - # __subclasses__ + # __subclasses__. exc_msg = "exception from __subclasses__" def raise_exc(): From 56b21e8a8c6b9ff7d5ec472975e288e3a1a58eee Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Mon, 20 Aug 2018 20:20:56 +0300 Subject: [PATCH 5/9] Test an invalid __subclasses__ signature with a valid return type --- Lib/test/test_abc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_abc.py b/Lib/test/test_abc.py index fc34207a01443e..e19c2661cecfe5 100644 --- a/Lib/test/test_abc.py +++ b/Lib/test/test_abc.py @@ -414,7 +414,7 @@ class C: # classes. bogus_subclasses = [ None, - lambda x: None, + lambda x: [], lambda: 42, lambda: [42], ] From 7df825eb9b7c8a4fb4f98cc1d2b233eddc114d3e Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Mon, 20 Aug 2018 20:23:38 +0300 Subject: [PATCH 6/9] Use subTest() to distinguish iterations --- Lib/test/test_abc.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_abc.py b/Lib/test/test_abc.py index e19c2661cecfe5..177278d3984730 100644 --- a/Lib/test/test_abc.py +++ b/Lib/test/test_abc.py @@ -419,11 +419,12 @@ class C: lambda: [42], ] - for bs in bogus_subclasses: + for i, bs in enumerate(bogus_subclasses): class S(metaclass=abc_ABCMeta): __subclasses__ = bs - with self.assertRaises(TypeError): + with self.subTest(i=i), \ + self.assertRaises(TypeError): issubclass(int, S) # Also check that issubclass() propagates exceptions raised by From dcf2e04a8ebc0d683d43f0e36f0adf43ec36d8d8 Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Mon, 20 Aug 2018 20:35:53 +0300 Subject: [PATCH 7/9] Generalize NEWS entry --- .../next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst b/Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst index 1a74cfca46e626..6db095bdf0c67c 100644 --- a/Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst +++ b/Misc/NEWS.d/next/Library/2018-08-20-16-48-32.bpo-34441._zx9lU.rst @@ -1,3 +1,3 @@ -Fix crash when an ``ABC``-derived class with non-callable ``__subclasses__`` -is passed as the second argument to :func:`issubclass()`. Patch by Alexey +Fix crash when an ``ABC``-derived class with invalid ``__subclasses__`` is +passed as the second argument to :func:`issubclass()`. Patch by Alexey Izbyshev. From 7d3c5ec097f3ba75c17a9bf56ca9d6ae1412b385 Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Mon, 20 Aug 2018 20:37:04 +0300 Subject: [PATCH 8/9] bs -> func --- Lib/test/test_abc.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_abc.py b/Lib/test/test_abc.py index 177278d3984730..8fc2f4cdde2fd4 100644 --- a/Lib/test/test_abc.py +++ b/Lib/test/test_abc.py @@ -419,9 +419,9 @@ class C: lambda: [42], ] - for i, bs in enumerate(bogus_subclasses): + for i, func in enumerate(bogus_subclasses): class S(metaclass=abc_ABCMeta): - __subclasses__ = bs + __subclasses__ = func with self.subTest(i=i), \ self.assertRaises(TypeError): From 72cb03187d272c48d8b6c0f547cbdb615caa8127 Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Mon, 20 Aug 2018 20:51:17 +0300 Subject: [PATCH 9/9] Use nested with statements for readability --- Lib/test/test_abc.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_abc.py b/Lib/test/test_abc.py index 8fc2f4cdde2fd4..9f5afb241aea3a 100644 --- a/Lib/test/test_abc.py +++ b/Lib/test/test_abc.py @@ -423,9 +423,9 @@ class C: class S(metaclass=abc_ABCMeta): __subclasses__ = func - with self.subTest(i=i), \ - self.assertRaises(TypeError): - issubclass(int, S) + with self.subTest(i=i): + with self.assertRaises(TypeError): + issubclass(int, S) # Also check that issubclass() propagates exceptions raised by # __subclasses__.