gh-104614: Make Sure ob_type is Always Set Correctly by PyType_Ready() (gh-105122)

When I added the relevant condition to type_ready_set_bases() in gh-103912, I had missed that the function also sets tp_base and ob_type (if necessary).  That led to problems for third-party static types.

We fix that here, by making those extra operations distinct and by adjusting the condition to be more specific.

Author: ericsnowcurrently

Lib/test/test_capi/test_misc.py

@@ -1,8 +1,9 @@
 # Run the _testcapi module tests (tests for the Python/C API):  by defn,
 # these are all functions _testcapi exports whose name begins with 'test_'.
 
-from collections import OrderedDict
 import _thread
+from collections import OrderedDict
+import contextlib
 import importlib.machinery
 import importlib.util
 import os
@@ -1626,6 +1627,41 @@ def test_tp_mro_is_set(self):
                 self.assertIsNot(mro, None)
 
 
+class TestStaticTypes(unittest.TestCase):
+
+    _has_run = False
+
+    @classmethod
+    def setUpClass(cls):
+        # The tests here don't play nice with our approach to refleak
+        # detection, so we bail out in that case.
+        if cls._has_run:
+            raise unittest.SkipTest('these tests do not support re-running')
+        cls._has_run = True
+
+    @contextlib.contextmanager
+    def basic_static_type(self, *args):
+        cls = _testcapi.get_basic_static_type(*args)
+        yield cls
+
+    def test_pytype_ready_always_sets_tp_type(self):
+        # The point of this test is to prevent something like
+        # https://github.com/python/cpython/issues/104614
+        # from happening again.
+
+        # First check when tp_base/tp_bases is *not* set before PyType_Ready().
+        with self.basic_static_type() as cls:
+            self.assertIs(cls.__base__, object);
+            self.assertEqual(cls.__bases__, (object,));
+            self.assertIs(type(cls), type(object));
+
+        # Then check when we *do* set tp_base/tp_bases first.
+        with self.basic_static_type(object) as cls:
+            self.assertIs(cls.__base__, object);
+            self.assertEqual(cls.__bases__, (object,));
+            self.assertIs(type(cls), type(object));
+
+
 class TestThreadState(unittest.TestCase):
 
     @threading_helper.reap_threads

Modules/_testcapimodule.c

@@ -2640,6 +2640,50 @@ type_get_tp_mro(PyObject *self, PyObject *type)
 }
 
 
+/* We only use 2 in test_capi/test_misc.py. */
+#define NUM_BASIC_STATIC_TYPES 2
+static PyTypeObject BasicStaticTypes[NUM_BASIC_STATIC_TYPES] = {
+#define INIT_BASIC_STATIC_TYPE \
+    { \
+        PyVarObject_HEAD_INIT(NULL, 0) \
+        .tp_name = "BasicStaticType", \
+        .tp_basicsize = sizeof(PyObject), \
+    }
+    INIT_BASIC_STATIC_TYPE,
+    INIT_BASIC_STATIC_TYPE,
+#undef INIT_BASIC_STATIC_TYPE
+};
+static int num_basic_static_types_used = 0;
+
+static PyObject *
+get_basic_static_type(PyObject *self, PyObject *args)
+{
+    PyObject *base = NULL;
+    if (!PyArg_ParseTuple(args, "|O", &base)) {
+        return NULL;
+    }
+    assert(base == NULL || PyType_Check(base));
+
+    if(num_basic_static_types_used >= NUM_BASIC_STATIC_TYPES) {
+        PyErr_SetString(PyExc_RuntimeError, "no more available basic static types");
+        return NULL;
+    }
+    PyTypeObject *cls = &BasicStaticTypes[num_basic_static_types_used++];
+
+    if (base != NULL) {
+        cls->tp_base = (PyTypeObject *)Py_NewRef(base);
+        cls->tp_bases = Py_BuildValue("(O)", base);
+        if (cls->tp_bases == NULL) {
+            return NULL;
+        }
+    }
+    if (PyType_Ready(cls) < 0) {
+        return NULL;
+    }
+    return (PyObject *)cls;
+}
+
+
 // Test PyThreadState C API
 static PyObject *
 test_tstate_capi(PyObject *self, PyObject *Py_UNUSED(args))
@@ -3395,6 +3439,7 @@ static PyMethodDef TestMethods[] = {
     {"type_assign_version", type_assign_version, METH_O, PyDoc_STR("PyUnstable_Type_AssignVersionTag")},
     {"type_get_tp_bases", type_get_tp_bases, METH_O},
     {"type_get_tp_mro", type_get_tp_mro, METH_O},
+    {"get_basic_static_type", get_basic_static_type, METH_VARARGS, NULL},
     {"test_tstate_capi", test_tstate_capi, METH_NOARGS, NULL},
     {"frame_getlocals", frame_getlocals, METH_O, NULL},
     {"frame_getglobals", frame_getglobals, METH_O, NULL},

Objects/typeobject.c

@@ -306,11 +306,14 @@ clear_tp_bases(PyTypeObject *self)
 {
     if (self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) {
         if (_Py_IsMainInterpreter(_PyInterpreterState_GET())) {
-            if (self->tp_bases != NULL
-                && PyTuple_GET_SIZE(self->tp_bases) > 0)
-            {
-                assert(_Py_IsImmortal(self->tp_bases));
-                _Py_ClearImmortal(self->tp_bases);
+            if (self->tp_bases != NULL) {
+                if (PyTuple_GET_SIZE(self->tp_bases) == 0) {
+                    Py_CLEAR(self->tp_bases);
+                }
+                else {
+                    assert(_Py_IsImmortal(self->tp_bases));
+                    _Py_ClearImmortal(self->tp_bases);
+                }
             }
         }
         return;
@@ -352,11 +355,14 @@ clear_tp_mro(PyTypeObject *self)
 {
     if (self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) {
         if (_Py_IsMainInterpreter(_PyInterpreterState_GET())) {
-            if (self->tp_mro != NULL
-                && PyTuple_GET_SIZE(self->tp_mro) > 0)
-            {
-                assert(_Py_IsImmortal(self->tp_mro));
-                _Py_ClearImmortal(self->tp_mro);
+            if (self->tp_mro != NULL) {
+                if (PyTuple_GET_SIZE(self->tp_mro) == 0) {
+                    Py_CLEAR(self->tp_mro);
+                }
+                else {
+                    assert(_Py_IsImmortal(self->tp_mro));
+                    _Py_ClearImmortal(self->tp_mro);
+                }
             }
         }
         return;
@@ -6996,12 +7002,8 @@ type_ready_pre_checks(PyTypeObject *type)
 
 
 static int
-type_ready_set_bases(PyTypeObject *type)
+type_ready_set_base(PyTypeObject *type)
 {
-    if (lookup_tp_bases(type) != NULL) {
-        return 0;
-    }
-
     /* Initialize tp_base (defaults to BaseObject unless that's us) */
     PyTypeObject *base = type->tp_base;
     if (base == NULL && type != &PyBaseObject_Type) {
@@ -7025,18 +7027,38 @@ type_ready_set_bases(PyTypeObject *type)
         }
     }
 
+    return 0;
+}
+
+static int
+type_ready_set_type(PyTypeObject *type)
+{
     /* Initialize ob_type if NULL.      This means extensions that want to be
        compilable separately on Windows can call PyType_Ready() instead of
        initializing the ob_type field of their type objects. */
     /* The test for base != NULL is really unnecessary, since base is only
        NULL when type is &PyBaseObject_Type, and we know its ob_type is
        not NULL (it's initialized to &PyType_Type).      But coverity doesn't
        know that. */
+    PyTypeObject *base = type->tp_base;
     if (Py_IS_TYPE(type, NULL) && base != NULL) {
         Py_SET_TYPE(type, Py_TYPE(base));
     }
 
-    /* Initialize tp_bases */
+    return 0;
+}
+
+static int
+type_ready_set_bases(PyTypeObject *type)
+{
+    if (type->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) {
+        if (!_Py_IsMainInterpreter(_PyInterpreterState_GET())) {
+            assert(lookup_tp_bases(type) != NULL);
+            return 0;
+        }
+        assert(lookup_tp_bases(type) == NULL);
+    }
+
     PyObject *bases = lookup_tp_bases(type);
     if (bases == NULL) {
         PyTypeObject *base = type->tp_base;
@@ -7446,6 +7468,12 @@ type_ready(PyTypeObject *type, int rerunbuiltin)
     if (type_ready_set_dict(type) < 0) {
         goto error;
     }
+    if (type_ready_set_base(type) < 0) {
+        goto error;
+    }
+    if (type_ready_set_type(type) < 0) {
+        goto error;
+    }
     if (type_ready_set_bases(type) < 0) {
         goto error;
     }

Tools/c-analyzer/cpython/ignored.tsv

@@ -420,6 +420,8 @@ Modules/_testcapi/watchers.c	-	num_code_object_destroyed_events	-
 Modules/_testcapi/watchers.c	-	pyfunc_watchers	-
 Modules/_testcapi/watchers.c	-	func_watcher_ids	-
 Modules/_testcapi/watchers.c	-	func_watcher_callbacks	-
+Modules/_testcapimodule.c	-	BasicStaticTypes	-
+Modules/_testcapimodule.c	-	num_basic_static_types_used	-
 Modules/_testcapimodule.c	-	ContainerNoGC_members	-
 Modules/_testcapimodule.c	-	ContainerNoGC_type	-
 Modules/_testcapimodule.c	-	FmData	-