From 146939306adcff706ebddb047f7470d148125cdf Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 1 Jun 2023 16:28:31 -0600 Subject: [PATCH] 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. --- Lib/test/test_capi/test_misc.py | 38 +++++++++++++++++- Modules/_testcapimodule.c | 45 +++++++++++++++++++++ Objects/typeobject.c | 60 ++++++++++++++++++++-------- Tools/c-analyzer/cpython/ignored.tsv | 2 + 4 files changed, 128 insertions(+), 17 deletions(-) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 037c8112d53e7a..e1b55cffe8ff52 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/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 diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index d7c89f48f792ed..38c2d1a91af31c 100644 --- a/Modules/_testcapimodule.c +++ b/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}, diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 6540d4ef7fc482..b6771d3004df91 100644 --- a/Objects/typeobject.c +++ b/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,6 +7027,12 @@ 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. */ @@ -7032,11 +7040,25 @@ type_ready_set_bases(PyTypeObject *type) 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; } diff --git a/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv index 5b08c74e1de51d..45f895dea02a44 100644 --- a/Tools/c-analyzer/cpython/ignored.tsv +++ b/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 -