Close #14199: _PyType_Lookup() and super_getattro() keep a strong reference to

the type MRO to avoid a crash if the MRO is changed during the lookup.

Lib/test/crashers/losing_mro_ref.py

@@ -1,35 +0,0 @@
-"""
-There is a way to put keys of any type in a type's dictionary.
-I think this allows various kinds of crashes, but so far I have only
-found a convoluted attack of _PyType_Lookup(), which uses the mro of the
-type without holding a strong reference to it.  Probably works with
-super.__getattribute__() too, which uses the same kind of code.
-"""
-
-class MyKey(object):
-    def __hash__(self):
-        return hash('mykey')
-
-    def __eq__(self, other):
-        # the following line decrefs the previous X.__mro__
-        X.__bases__ = (Base2,)
-        # trash all tuples of length 3, to make sure that the items of
-        # the previous X.__mro__ are really garbage
-        z = []
-        for i in range(1000):
-            z.append((i, None, None))
-        return 0
-
-
-class Base(object):
-    mykey = 'from Base'
-
-class Base2(object):
-    mykey = 'from Base2'
-
-# you can't add a non-string key to X.__dict__, but it can be
-# there from the beginning :-)
-X = type('X', (Base,), {MyKey(): 5})
-
-print(X.mykey)
-# I get a segfault, or a slightly wrong assertion error in a debug build.

Lib/test/test_descr.py

@@ -4582,10 +4582,38 @@ class PTypesLongInitTest(unittest.TestCase):
         type.mro(tuple)
 
 
+class MiscTests(unittest.TestCase):
+    def test_type_lookup_mro_reference(self):
+        # Issue #14199: _PyType_Lookup() has to keep a strong reference to
+        # the type MRO because it may be modified during the lookup, if
+        # __bases__ is set during the lookup for example.
+        class MyKey(object):
+            def __hash__(self):
+                return hash('mykey')
+
+            def __eq__(self, other):
+                X.__bases__ = (Base2,)
+
+        class Base(object):
+            mykey = 'from Base'
+            mykey2 = 'from Base'
+
+        class Base2(object):
+            mykey = 'from Base2'
+            mykey2 = 'from Base2'
+
+        X = type('X', (Base,), {MyKey(): 5})
+        # mykey is read from Base
+        self.assertEqual(X.mykey, 'from Base')
+        # mykey2 is read from Base2 because MyKey.__eq__ has set __bases__
+        self.assertEqual(X.mykey2, 'from Base2')
+
+
 def test_main():
     # Run all local test cases, with PTypesLongInitTest first.
     support.run_unittest(PTypesLongInitTest, OperatorsTest,
-                              ClassPropertiesAndMethods, DictProxyTests)
+                         ClassPropertiesAndMethods, DictProxyTests,
+                         MiscTests)
 
 if __name__ == "__main__":
     test_main()

Objects/typeobject.c

@@ -2450,6 +2450,9 @@ _PyType_Lookup(PyTypeObject *type, PyObject *name)
         return NULL;
 
     res = NULL;
+    /* keep a strong reference to mro because type->tp_mro can be replaced
+       during PyDict_GetItem(dict, name)  */
+    Py_INCREF(mro);
     assert(PyTuple_Check(mro));
     n = PyTuple_GET_SIZE(mro);
     for (i = 0; i < n; i++) {
@@ -2461,6 +2464,7 @@ _PyType_Lookup(PyTypeObject *type, PyObject *name)
         if (res != NULL)
             break;
     }
+    Py_DECREF(mro);
 
     if (MCACHE_CACHEABLE_NAME(name) && assign_version_tag(type)) {
         h = MCACHE_HASH_METHOD(type, name);
@@ -6281,6 +6285,9 @@ super_getattro(PyObject *self, PyObject *name)
         }
         i++;
         res = NULL;
+        /* keep a strong reference to mro because starttype->tp_mro can be
+           replaced during PyDict_GetItem(dict, name)  */
+        Py_INCREF(mro);
         for (; i < n; i++) {
             tmp = PyTuple_GET_ITEM(mro, i);
             if (PyType_Check(tmp))
@@ -6305,9 +6312,11 @@ super_getattro(PyObject *self, PyObject *name)
                     Py_DECREF(res);
                     res = tmp;
                 }
+                Py_DECREF(mro);
                 return res;
             }
         }
+        Py_DECREF(mro);
     }
     return PyObject_GenericGetAttr(self, name);
 }