bpo-39382: Avoid dangling object use in abstract_issubclass() (GH-18530)

Hold reference of __bases__ tuple until tuple item is done with, because by
dropping the reference the item may be destroyed.
(cherry picked from commit 1c56f8ffad44478b4214a2bf8eb7cf51c28a347a)

Co-authored-by: Yonatan Goldschmidt <yon.goldschmidt@gmail.com>

diff --git a/Lib/test/test_isinstance.py b/Lib/test/test_isinstance.py
index 65751ab91685503ae4d8b9baf533c06e0c4374b3..53639e984e48a784cff38339c725ee1b9711be4e 100644 (file)
--- a/Lib/test/test_isinstance.py
+++ b/Lib/test/test_isinstance.py
@@ -251,6 +251,27 @@ class TestIsInstanceIsSubclass(unittest.TestCase):
         # blown
         self.assertRaises(RecursionError, blowstack, isinstance, '', str)
 
+    def test_issubclass_refcount_handling(self):
+        # bpo-39382: abstract_issubclass() didn't hold item reference while
+        # peeking in the bases tuple, in the single inheritance case.
+        class A:
+            @property
+            def __bases__(self):
+                return (int, )
+
+        class B:
+            def __init__(self):
+                # setting this here increases the chances of exhibiting the bug,
+                # probably due to memory layout changes.
+                self.x = 1
+
+            @property
+            def __bases__(self):
+                return (A(), )
+
+        self.assertEqual(True, issubclass(B(), int))
+
+
 def blowstack(fxn, arg, compare_to):
     # Make sure that calling isinstance with a deeply nested tuple for its
     # argument will raise RecursionError eventually.

diff --git a/Misc/ACKS b/Misc/ACKS
index 613763195b68785b421d57a00a497624906d28b5..2107435cf093b481bb1193af819b5294934c8a8e 100644 (file)
--- a/Misc/ACKS
+++ b/Misc/ACKS
@@ -568,6 +568,7 @@ Karan Goel
 Jeroen Van Goey
 Christoph Gohlke
 Tim Golden
+Yonatan Goldschmidt
 Mark Gollahon
 Guilherme Gonçalves
 Tiago Gonçalves

diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-02-18-01-40-13.bpo-39382.OLSJu9.rst b/Misc/NEWS.d/next/Core and Builtins/2020-02-18-01-40-13.bpo-39382.OLSJu9.rst
new file mode 100644 (file)
index 0000000..605f4c8
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2020-02-18-01-40-13.bpo-39382.OLSJu9.rst
@@ -0,0 +1,3 @@
+Fix a use-after-free in the single inheritance path of ``issubclass()``, when
+the ``__bases__`` of an object has a single reference, and so does its first item.
+Patch by Yonatan Goldschmidt.

diff --git a/Objects/abstract.c b/Objects/abstract.c
index 9416df4321f3f9e795797ad1a5210f22761b1c46..e6831feb82433178dff215dd67a99cfdbe2f2353 100644 (file)
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -2287,9 +2287,16 @@ abstract_issubclass(PyObject *derived, PyObject *cls)
     int r = 0;
 
     while (1) {
-        if (derived == cls)
+        if (derived == cls) {
+            Py_XDECREF(bases); /* See below comment */
             return 1;
-        bases = abstract_get_bases(derived);
+        }
+        /* Use XSETREF to drop bases reference *after* finishing with
+           derived; bases might be the only reference to it.
+           XSETREF is used instead of SETREF, because bases is NULL on the
+           first iteration of the loop.
+        */
+        Py_XSETREF(bases, abstract_get_bases(derived));
         if (bases == NULL) {
             if (PyErr_Occurred())
                 return -1;
@@ -2303,7 +2310,6 @@ abstract_issubclass(PyObject *derived, PyObject *cls)
         /* Avoid recursivity in the single inheritance case */
         if (n == 1) {
             derived = PyTuple_GET_ITEM(bases, 0);
-            Py_DECREF(bases);
             continue;
         }
         for (i = 0; i < n; i++) {