From fe12a9026fa69e4eab6347b0ad810041839393af Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 30 May 2023 16:54:41 -0600 Subject: [PATCH 1/6] Split up type_ready_set_bases(). --- Objects/typeobject.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 6540d4ef7fc482..a888487fabfee0 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -6996,12 +6996,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 +7021,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 +7034,21 @@ 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 (lookup_tp_bases(type) != NULL) { + return 0; + } + PyObject *bases = lookup_tp_bases(type); if (bases == NULL) { PyTypeObject *base = type->tp_base; @@ -7446,6 +7458,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; } From 86f8ec6496b31b1cee07850e11be06a6cbdae794 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 30 May 2023 16:55:47 -0600 Subject: [PATCH 2/6] Fix the short-circuit in type_ready_set_bases(). --- Objects/typeobject.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index a888487fabfee0..83bb76ccce7a16 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -7045,8 +7045,12 @@ type_ready_set_type(PyTypeObject *type) static int type_ready_set_bases(PyTypeObject *type) { - if (lookup_tp_bases(type) != NULL) { - return 0; + 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); From e4337e2f30af2c56c6ae7f0654d624343529a962 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 30 May 2023 19:28:42 -0600 Subject: [PATCH 3/6] Fix the empty tuple case for clear_tp_bases(). --- Objects/typeobject.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 83bb76ccce7a16..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; From b6801173ad01cf025ed49774d849ef7be0d29ff5 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 31 May 2023 17:12:02 -0600 Subject: [PATCH 4/6] Add a regression test. --- Lib/test/test_capi/test_misc.py | 31 +++++++++++++++++- Modules/_testcapimodule.c | 56 +++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 037c8112d53e7a..8a04a37458d286 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,34 @@ def test_tp_mro_is_set(self): self.assertIsNot(mro, None) +class TestStaticTypes(unittest.TestCase): + + 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. + + @contextlib.contextmanager + def basic_static_type(*args): + cls = _testcapi.get_basic_static_type(*args) + try: + yield cls + finally: + _testcapi.clear_basic_static_type(cls) + + # First check when tp_base/tp_bases is *not* set before PyType_Ready(). + with 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 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 66c1cbabe0f8c4..96f2f5476b82ae 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -2627,6 +2627,60 @@ type_get_tp_mro(PyObject *self, PyObject *type) } +static PyTypeObject BasicStaticType = { + PyVarObject_HEAD_INIT(NULL, 0) + .tp_name = "BasicStaticType", + .tp_basicsize = sizeof(PyObject), +}; + +static PyObject * clear_basic_static_type(PyObject *, PyObject *); + +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)); + + PyTypeObject *cls = &BasicStaticType; + assert(!(cls->tp_flags & Py_TPFLAGS_READY)); + + if (base != NULL) { + cls->tp_base = (PyTypeObject *)Py_NewRef(base); + cls->tp_bases = Py_BuildValue("(O)", base); + if (cls->tp_bases == NULL) { + clear_basic_static_type(self, (PyObject *)cls); + return NULL; + } + } + if (PyType_Ready(cls) < 0) { + clear_basic_static_type(self, (PyObject *)cls); + return NULL; + } + Py_INCREF(cls); + return (PyObject *)cls; +} + +static PyObject * +clear_basic_static_type(PyObject *self, PyObject *clsobj) +{ + // Reset it back to the statically initialized state. + PyTypeObject *cls = (PyTypeObject *)clsobj; + Py_CLEAR(cls->ob_base.ob_base.ob_type); + Py_CLEAR(cls->tp_base); + Py_CLEAR(cls->tp_bases); + Py_CLEAR(cls->tp_mro); + Py_CLEAR(cls->tp_subclasses); + Py_CLEAR(cls->tp_dict); + cls->tp_flags &= ~Py_TPFLAGS_READY; + cls->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; + cls->tp_version_tag = 0; + Py_RETURN_NONE; +} + + // Test PyThreadState C API static PyObject * test_tstate_capi(PyObject *self, PyObject *Py_UNUSED(args)) @@ -3384,6 +3438,8 @@ 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}, + {"clear_basic_static_type", clear_basic_static_type, METH_O, NULL}, {"test_tstate_capi", test_tstate_capi, METH_NOARGS, NULL}, {"frame_getlocals", frame_getlocals, METH_O, NULL}, {"frame_getglobals", frame_getglobals, METH_O, NULL}, From 08a0321b3578c5b7749dd46277a745e86366aa34 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 31 May 2023 17:32:40 -0600 Subject: [PATCH 5/6] Ignore the test static type. --- Tools/c-analyzer/cpython/ignored.tsv | 1 + 1 file changed, 1 insertion(+) diff --git a/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv index 5b08c74e1de51d..b63691d441d031 100644 --- a/Tools/c-analyzer/cpython/ignored.tsv +++ b/Tools/c-analyzer/cpython/ignored.tsv @@ -420,6 +420,7 @@ 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 - BasicStaticType - Modules/_testcapimodule.c - ContainerNoGC_members - Modules/_testcapimodule.c - ContainerNoGC_type - Modules/_testcapimodule.c - FmData - From 80d20c650d5d758a65c7b824c729e55d7f2693bb Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 1 Jun 2023 14:40:01 -0600 Subject: [PATCH 6/6] Fix the test. --- Lib/test/test_capi/test_misc.py | 27 ++++++++++------ Modules/_testcapimodule.c | 47 +++++++++++----------------- Tools/c-analyzer/cpython/ignored.tsv | 3 +- 3 files changed, 37 insertions(+), 40 deletions(-) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 8a04a37458d286..e1b55cffe8ff52 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1629,27 +1629,34 @@ def test_tp_mro_is_set(self): 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. - @contextlib.contextmanager - def basic_static_type(*args): - cls = _testcapi.get_basic_static_type(*args) - try: - yield cls - finally: - _testcapi.clear_basic_static_type(cls) - # First check when tp_base/tp_bases is *not* set before PyType_Ready(). - with basic_static_type() as cls: + 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 basic_static_type(object) as cls: + with self.basic_static_type(object) as cls: self.assertIs(cls.__base__, object); self.assertEqual(cls.__bases__, (object,)); self.assertIs(type(cls), type(object)); diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 96f2f5476b82ae..3caaca35cd742d 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -2627,13 +2627,20 @@ type_get_tp_mro(PyObject *self, PyObject *type) } -static PyTypeObject BasicStaticType = { - PyVarObject_HEAD_INIT(NULL, 0) - .tp_name = "BasicStaticType", - .tp_basicsize = sizeof(PyObject), +/* 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 PyObject * clear_basic_static_type(PyObject *, PyObject *); +static int num_basic_static_types_used = 0; static PyObject * get_basic_static_type(PyObject *self, PyObject *args) @@ -2644,42 +2651,25 @@ get_basic_static_type(PyObject *self, PyObject *args) } assert(base == NULL || PyType_Check(base)); - PyTypeObject *cls = &BasicStaticType; - assert(!(cls->tp_flags & Py_TPFLAGS_READY)); + 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) { - clear_basic_static_type(self, (PyObject *)cls); return NULL; } } if (PyType_Ready(cls) < 0) { - clear_basic_static_type(self, (PyObject *)cls); return NULL; } - Py_INCREF(cls); return (PyObject *)cls; } -static PyObject * -clear_basic_static_type(PyObject *self, PyObject *clsobj) -{ - // Reset it back to the statically initialized state. - PyTypeObject *cls = (PyTypeObject *)clsobj; - Py_CLEAR(cls->ob_base.ob_base.ob_type); - Py_CLEAR(cls->tp_base); - Py_CLEAR(cls->tp_bases); - Py_CLEAR(cls->tp_mro); - Py_CLEAR(cls->tp_subclasses); - Py_CLEAR(cls->tp_dict); - cls->tp_flags &= ~Py_TPFLAGS_READY; - cls->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; - cls->tp_version_tag = 0; - Py_RETURN_NONE; -} - // Test PyThreadState C API static PyObject * @@ -3439,7 +3429,6 @@ static PyMethodDef TestMethods[] = { {"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}, - {"clear_basic_static_type", clear_basic_static_type, METH_O, 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/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv index b63691d441d031..45f895dea02a44 100644 --- a/Tools/c-analyzer/cpython/ignored.tsv +++ b/Tools/c-analyzer/cpython/ignored.tsv @@ -420,7 +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 - BasicStaticType - +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 -