From d43d5ca0914e1ec67c136c421293f93c16a05206 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 31 May 2022 16:06:42 +0200 Subject: [PATCH 01/18] Raise invalid slot exceptions early --- Objects/typeobject.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index e57651446f888f..413832e877a7ea 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3416,6 +3416,11 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, nmembers = weaklistoffset = dictoffset = vectorcalloffset = 0; for (slot = spec->slots; slot->slot; slot++) { + if (slot->slot < 0 + || (size_t)slot->slot >= Py_ARRAY_LENGTH(pyslot_offsets)) { + PyErr_SetString(PyExc_RuntimeError, "invalid slot offset"); + goto finally; + } if (slot->slot == Py_tp_members) { if (nmembers != 0) { PyErr_SetString( @@ -3553,12 +3558,7 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, type->tp_itemsize = spec->itemsize; for (slot = spec->slots; slot->slot; slot++) { - if (slot->slot < 0 - || (size_t)slot->slot >= Py_ARRAY_LENGTH(pyslot_offsets)) { - PyErr_SetString(PyExc_RuntimeError, "invalid slot offset"); - goto finally; - } - else if (slot->slot == Py_tp_base || slot->slot == Py_tp_bases) { + if (slot->slot == Py_tp_base || slot->slot == Py_tp_bases) { /* Processed above */ continue; } From 5145034e7f920e52baf719649d0d2ec9f2aece4f Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 31 May 2022 16:11:46 +0200 Subject: [PATCH 02/18] Switch to `switch` for the slots --- Objects/typeobject.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 413832e877a7ea..11c4f487c0aeb3 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3421,7 +3421,8 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, PyErr_SetString(PyExc_RuntimeError, "invalid slot offset"); goto finally; } - if (slot->slot == Py_tp_members) { + switch (slot->slot) { + case Py_tp_members: if (nmembers != 0) { PyErr_SetString( PyExc_SystemError, @@ -3449,6 +3450,7 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, vectorcalloffset = memb->offset; } } + break; } } @@ -3558,11 +3560,13 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, type->tp_itemsize = spec->itemsize; for (slot = spec->slots; slot->slot; slot++) { - if (slot->slot == Py_tp_base || slot->slot == Py_tp_bases) { + size_t len; + switch (slot->slot) { + case Py_tp_base: + case Py_tp_bases: /* Processed above */ - continue; - } - else if (slot->slot == Py_tp_doc) { + break; + case Py_tp_doc: /* For the docstring slot, which usually points to a static string literal, we need to make a copy */ if (type->tp_doc != NULL) { @@ -3573,9 +3577,9 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, } if (slot->pfunc == NULL) { type->tp_doc = NULL; - continue; + break; } - size_t len = strlen(slot->pfunc)+1; + len = strlen(slot->pfunc)+1; char *tp_doc = PyObject_Malloc(len); if (tp_doc == NULL) { type->tp_doc = NULL; @@ -3584,14 +3588,14 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, } memcpy(tp_doc, slot->pfunc, len); type->tp_doc = tp_doc; - } - else if (slot->slot == Py_tp_members) { + break; + case Py_tp_members: /* Move the slots to the heap type itself */ - size_t len = Py_TYPE(type)->tp_itemsize * nmembers; + len = Py_TYPE(type)->tp_itemsize * nmembers; memcpy(_PyHeapType_GET_MEMBERS(res), slot->pfunc, len); type->tp_members = _PyHeapType_GET_MEMBERS(res); - } - else { + break; + default: /* Copy other slots directly */ PySlot_Offset slotoffsets = pyslot_offsets[slot->slot]; slot_offset = slotoffsets.slot_offset; @@ -3602,6 +3606,7 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, subslot_offset = slotoffsets.subslot_offset; *(void**)((char*)parent_slot + subslot_offset) = slot->pfunc; } + break; } } if (type->tp_dealloc == NULL) { From 87b28aa319e0ee2b0c925e4efdc21400a2b9477b Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 1 Jun 2022 16:33:16 +0200 Subject: [PATCH 03/18] Allocate & copy the docstring early --- Objects/typeobject.c | 55 ++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 11c4f487c0aeb3..be535af673a128 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3406,6 +3406,7 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, PyObject *modname = NULL; PyTypeObject *type; PyObject *bases = NULL; + char *tp_doc = NULL; int r; const PyType_Slot *slot; @@ -3451,6 +3452,29 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, } } break; + case Py_tp_doc: + /* For the docstring slot, which usually points to a static string + literal, we need to make a copy */ + if (tp_doc != NULL) { + PyErr_SetString( + PyExc_SystemError, + "Multiple Py_tp_doc slots are not supported."); + return NULL; + } + if (slot->pfunc == NULL) { + PyObject_Free(tp_doc); + tp_doc = NULL; + } + else { + size_t len = strlen(slot->pfunc)+1; + tp_doc = PyObject_Malloc(len); + if (tp_doc == NULL) { + PyErr_NoMemory(); + goto finally; + } + memcpy(tp_doc, slot->pfunc, len); + } + break; } } @@ -3555,43 +3579,23 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, type->tp_bases = bases; bases = NULL; // We give our reference to bases to the type + type->tp_doc = tp_doc; + tp_doc = NULL; // Give ownership of the allocated memory to the type + /* Copy the sizes */ type->tp_basicsize = spec->basicsize; type->tp_itemsize = spec->itemsize; for (slot = spec->slots; slot->slot; slot++) { - size_t len; switch (slot->slot) { case Py_tp_base: case Py_tp_bases: - /* Processed above */ - break; case Py_tp_doc: - /* For the docstring slot, which usually points to a static string - literal, we need to make a copy */ - if (type->tp_doc != NULL) { - PyErr_SetString( - PyExc_SystemError, - "Multiple Py_tp_doc slots are not supported."); - return NULL; - } - if (slot->pfunc == NULL) { - type->tp_doc = NULL; - break; - } - len = strlen(slot->pfunc)+1; - char *tp_doc = PyObject_Malloc(len); - if (tp_doc == NULL) { - type->tp_doc = NULL; - PyErr_NoMemory(); - goto finally; - } - memcpy(tp_doc, slot->pfunc, len); - type->tp_doc = tp_doc; + /* Processed above */ break; case Py_tp_members: /* Move the slots to the heap type itself */ - len = Py_TYPE(type)->tp_itemsize * nmembers; + size_t len = Py_TYPE(type)->tp_itemsize * nmembers; memcpy(_PyHeapType_GET_MEMBERS(res), slot->pfunc, len); type->tp_members = _PyHeapType_GET_MEMBERS(res); break; @@ -3687,6 +3691,7 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, } Py_XDECREF(bases); Py_XDECREF(modname); + PyObject_Free(tp_doc); return (PyObject*)res; } From 9fb6b1109e03444d494d643982a62c0dee4ea4d5 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 1 Jun 2022 16:54:47 +0200 Subject: [PATCH 04/18] Prepare the type names early --- Objects/typeobject.c | 81 +++++++++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 32 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index be535af673a128..c079921823dee6 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3407,6 +3407,8 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, PyTypeObject *type; PyObject *bases = NULL; char *tp_doc = NULL; + PyObject *ht_name = NULL; + char *_ht_tpname = NULL; int r; const PyType_Slot *slot; @@ -3478,12 +3480,43 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, } } + /* Prepare the type name and qualname */ + if (spec->name == NULL) { PyErr_SetString(PyExc_SystemError, "Type spec does not define the name field."); goto finally; } + const char *s = strrchr(spec->name, '.'); + if (s == NULL) { + s = spec->name; + } + else { + s++; + } + + ht_name = PyUnicode_FromString(s); + if (!ht_name) { + goto finally; + } + + /* Copy spec->name to a buffer we own. + * + * Unfortunately, we can't use tp_name directly (with some + * flag saying that it should be deallocated with the type), + * because tp_name is public API and may be set independently + * of any such flag. + * So, we use a separate buffer, _ht_tpname, that's always + * deallocated with the type (if it's non-NULL). + */ + Py_ssize_t name_buf_len = strlen(spec->name) + 1; + _ht_tpname = PyMem_Malloc(name_buf_len); + if (_ht_tpname == NULL) { + goto finally; + } + memcpy(_ht_tpname, spec->name, name_buf_len); + /* Get a tuple of bases. * bases is a strong reference (unlike bases_in). */ @@ -3534,47 +3567,18 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, /* The flags must be initialized early, before the GC traverses us */ type->tp_flags = spec->flags | Py_TPFLAGS_HEAPTYPE; - /* Set the type name and qualname */ - const char *s = strrchr(spec->name, '.'); - if (s == NULL) { - s = spec->name; - } - else { - s++; - } - - res->ht_name = PyUnicode_FromString(s); - if (!res->ht_name) { - goto finally; - } - res->ht_qualname = Py_NewRef(res->ht_name); - - /* Copy spec->name to a buffer we own. - * - * Unfortunately, we can't use tp_name directly (with some - * flag saying that it should be deallocated with the type), - * because tp_name is public API and may be set independently - * of any such flag. - * So, we use a separate buffer, _ht_tpname, that's always - * deallocated with the type (if it's non-NULL). - */ - Py_ssize_t name_buf_len = strlen(spec->name) + 1; - res->_ht_tpname = PyMem_Malloc(name_buf_len); - if (res->_ht_tpname == NULL) { - goto finally; - } - type->tp_name = memcpy(res->_ht_tpname, spec->name, name_buf_len); - res->ht_module = Py_XNewRef(module); /* Initialize essential fields */ + type->tp_as_async = &res->as_async; type->tp_as_number = &res->as_number; type->tp_as_sequence = &res->as_sequence; type->tp_as_mapping = &res->as_mapping; type->tp_as_buffer = &res->as_buffer; - /* Set tp_base and tp_bases */ + /* Set slots we have prepared */ + type->tp_base = (PyTypeObject *)Py_NewRef(base); type->tp_bases = bases; bases = NULL; // We give our reference to bases to the type @@ -3582,10 +3586,21 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, type->tp_doc = tp_doc; tp_doc = NULL; // Give ownership of the allocated memory to the type + res->ht_qualname = Py_NewRef(ht_name); + res->ht_name = ht_name; + ht_name = NULL; // Give our reference to to the type + + type->tp_name = _ht_tpname; + res->_ht_tpname = _ht_tpname; + _ht_tpname = NULL; // Give ownership to to the type + /* Copy the sizes */ + type->tp_basicsize = spec->basicsize; type->tp_itemsize = spec->itemsize; + /* Copy all the ordinary slots */ + for (slot = spec->slots; slot->slot; slot++) { switch (slot->slot) { case Py_tp_base: @@ -3692,6 +3707,8 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, Py_XDECREF(bases); Py_XDECREF(modname); PyObject_Free(tp_doc); + Py_XDECREF(ht_name); + PyMem_Free(_ht_tpname); return (PyObject*)res; } From 7fd0c6da67030d56d4bbbb20055bcd342c8e75ac Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 1 Jun 2022 17:01:48 +0200 Subject: [PATCH 05/18] Move offset setting to before PyType_Ready --- Objects/typeobject.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index c079921823dee6..1ebf45e10d8592 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3635,9 +3635,17 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, type->tp_dealloc = subtype_dealloc; } - if (vectorcalloffset) { - type->tp_vectorcall_offset = vectorcalloffset; - } + /* Set up offsets */ + + type->tp_vectorcall_offset = vectorcalloffset; + type->tp_weaklistoffset = weaklistoffset; + type->tp_dictoffset = dictoffset; + + /* Ready the type (which includes inheritance). + * + * After this call we should generally only touch up what's + * accessible to Python code, like __dict__. + */ if (PyType_Ready(type) < 0) { goto finally; @@ -3660,13 +3668,11 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, } if (weaklistoffset) { - type->tp_weaklistoffset = weaklistoffset; if (PyDict_DelItemString((PyObject *)type->tp_dict, "__weaklistoffset__") < 0) { goto finally; } } if (dictoffset) { - type->tp_dictoffset = dictoffset; if (PyDict_DelItemString((PyObject *)type->tp_dict, "__dictoffset__") < 0) { goto finally; } From 06fe302f9bdd6737e8e28cfe4169e540ed3b7e5b Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 1 Jun 2022 16:37:54 +0200 Subject: [PATCH 06/18] Add comment on preparing special slots --- Objects/typeobject.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 1ebf45e10d8592..9830e670b96b7c 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3411,6 +3411,11 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, char *_ht_tpname = NULL; int r; + /* Prepare slots that need special handling. + * Keep in mind that a slot can be given multiple times: + * if that would cause trouble (leaks, UB, ...), raise an exception. + */ + const PyType_Slot *slot; Py_ssize_t nmembers = 0; Py_ssize_t weaklistoffset, dictoffset, vectorcalloffset; From 8bb54f861208d0d23646540e499a3b749341ca1b Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 1 Jun 2022 17:15:34 +0200 Subject: [PATCH 07/18] Comment that we don't want to call Python code while a type is half-built --- Objects/typeobject.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 9830e670b96b7c..84583b8ebdf53b 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3560,7 +3560,13 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, // here we just check its work assert(_PyType_HasFeature(base, Py_TPFLAGS_BASETYPE)); - /* Allocate the new type */ + /* Allocate the new type + * + * Between here and PyType_Ready, we should limit: + * - calls to Python code + * - raising exceptions + * - memory allocations + */ res = (PyHeapTypeObject*)metaclass->tp_alloc(metaclass, nmembers); if (res == NULL) { From 6c25cfcda973ed5cca2ed6c7daec34af3f5302db Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 1 Jun 2022 17:16:32 +0200 Subject: [PATCH 08/18] Add comment on the function's refcounting strategy --- Objects/typeobject.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 84583b8ebdf53b..bd0c6c4290b423 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3402,6 +3402,10 @@ PyObject * PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, PyType_Spec *spec, PyObject *bases_in) { + /* Invariant: A non-NULL value in one of these means this function holds + * a strong reference or owns allocated memory. + * These get decrefed/freed/returned at the end, on both success and error. + */ PyHeapTypeObject *res = NULL; PyObject *modname = NULL; PyTypeObject *type; @@ -3409,6 +3413,7 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, char *tp_doc = NULL; PyObject *ht_name = NULL; char *_ht_tpname = NULL; + int r; /* Prepare slots that need special handling. From 7edb47857f26cf70da3c60c6b63099d197cbdf88 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 1 Jun 2022 18:33:44 +0200 Subject: [PATCH 09/18] Check that base & offsets fit in the basicsize --- Modules/_testcapimodule.c | 3 ++- Objects/typeobject.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 33dc3dbe493c51..4881b045af3fad 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -1221,7 +1221,7 @@ static PyType_Spec MinimalMetaclass_spec = { static PyType_Spec MinimalType_spec = { .name = "_testcapi.MinimalSpecType", - .basicsize = sizeof(PyObject), + .basicsize = 0, // Updated later .flags = Py_TPFLAGS_DEFAULT, .slots = empty_type_slots, }; @@ -1245,6 +1245,7 @@ test_from_spec_metatype_inheritance(PyObject *self, PyObject *Py_UNUSED(ignored) goto finally; } + MinimalType_spec.basicsize = ((PyTypeObject*)class)->tp_basicsize; new = PyType_FromSpecWithBases(&MinimalType_spec, class); if (new == NULL) { goto finally; diff --git a/Objects/typeobject.c b/Objects/typeobject.c index bd0c6c4290b423..b81e90f8838392 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3398,6 +3398,41 @@ get_bases_tuple(PyObject *bases_in, PyType_Spec *spec) return PyTuple_Pack(1, bases_in); } +static inline int +check_basicsize_includes_size_and_offsets(PyTypeObject* type) { + if (type->tp_alloc != PyType_GenericAlloc) { + // Custom allocators can ignore tp_basicsize + return 1; + } + + if (type->tp_base && type->tp_base->tp_basicsize > type->tp_basicsize) { + PyErr_Format(PyExc_TypeError, + "tp_basicsize for type '%s' (%d) is too small for base '%s' (%d)", + type->tp_name, type->tp_basicsize, + type->tp_base->tp_name, type->tp_base->tp_basicsize); + return 0; + } + if (type->tp_weaklistoffset + (Py_ssize_t)sizeof(PyObject*) > (Py_ssize_t)type->tp_basicsize) { + PyErr_Format(PyExc_TypeError, + "tp_basicsize for type '%s' (%d) is too small for weaklist offset %d", + type->tp_name, type->tp_basicsize, type->tp_weaklistoffset); + return 0; + } + if (type->tp_dictoffset + (Py_ssize_t)sizeof(PyObject*) > (Py_ssize_t)type->tp_basicsize) { + PyErr_Format(PyExc_TypeError, + "tp_basicsize for type '%s' (%d) is too small for dict offset %d", + type->tp_name, type->tp_basicsize, type->tp_dictoffset); + return 0; + } + if (type->tp_vectorcall_offset + (Py_ssize_t)sizeof(vectorcallfunc*) > (Py_ssize_t)type->tp_basicsize) { + PyErr_Format(PyExc_TypeError, + "tp_basicsize for type '%s' (%d) is too small for vectorcall offset %d", + type->tp_name, type->tp_basicsize, type->tp_vectorcall_offset); + return 0; + } + return 1; +} + PyObject * PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, PyType_Spec *spec, PyObject *bases_in) @@ -3667,6 +3702,10 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, goto finally; } + if (!check_basicsize_includes_size_and_offsets(type)) { + goto finally; + } + if (type->tp_flags & Py_TPFLAGS_MANAGED_DICT) { res->ht_cached_keys = _PyDict_NewKeysForClass(); } From 2075958fd9c9421e39c51ebf5f67d1174bad4332 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 10 Jun 2022 16:50:30 +0200 Subject: [PATCH 10/18] Add blurb for the new check --- .../next/C API/2022-06-10-16-50-27.gh-issue-89546.mX1f10.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 Misc/NEWS.d/next/C API/2022-06-10-16-50-27.gh-issue-89546.mX1f10.rst diff --git a/Misc/NEWS.d/next/C API/2022-06-10-16-50-27.gh-issue-89546.mX1f10.rst b/Misc/NEWS.d/next/C API/2022-06-10-16-50-27.gh-issue-89546.mX1f10.rst new file mode 100644 index 00000000000000..1f9f9afab96552 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2022-06-10-16-50-27.gh-issue-89546.mX1f10.rst @@ -0,0 +1,4 @@ +:c:func`PyType_FromMetaclass` (and other ``PyType_From*`` functions) now +check that offsets and the base class's +:c:member:`~PyTypeObject.tp_basicsize` fit in the new class's +``tp_basicsize``. From 763ea2dfa843dec2e0af779e6c797ac791955a3c Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 10 Jun 2022 16:53:15 +0200 Subject: [PATCH 11/18] Move variable declarations out of `switch` cases --- Objects/typeobject.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index b81e90f8838392..343b6904709dc9 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3450,6 +3450,7 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, char *_ht_tpname = NULL; int r; + size_t len; /* Prepare slots that need special handling. * Keep in mind that a slot can be given multiple times: @@ -3513,7 +3514,7 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, tp_doc = NULL; } else { - size_t len = strlen(slot->pfunc)+1; + len = strlen(slot->pfunc)+1; tp_doc = PyObject_Malloc(len); if (tp_doc == NULL) { PyErr_NoMemory(); @@ -3653,6 +3654,7 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, /* Copy all the ordinary slots */ for (slot = spec->slots; slot->slot; slot++) { + PySlot_Offset slotoffsets; switch (slot->slot) { case Py_tp_base: case Py_tp_bases: @@ -3661,13 +3663,13 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, break; case Py_tp_members: /* Move the slots to the heap type itself */ - size_t len = Py_TYPE(type)->tp_itemsize * nmembers; + len = Py_TYPE(type)->tp_itemsize * nmembers; memcpy(_PyHeapType_GET_MEMBERS(res), slot->pfunc, len); type->tp_members = _PyHeapType_GET_MEMBERS(res); break; default: /* Copy other slots directly */ - PySlot_Offset slotoffsets = pyslot_offsets[slot->slot]; + slotoffsets = pyslot_offsets[slot->slot]; slot_offset = slotoffsets.slot_offset; if (slotoffsets.subslot_offset == -1) { *(void**)((char*)res_start + slot_offset) = slot->pfunc; From 5ca1502476c9588177c620151b79531813f4479b Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 10 Jun 2022 17:05:09 +0200 Subject: [PATCH 12/18] Fix ReST syntax (*sigh*) --- .../next/C API/2022-06-10-16-50-27.gh-issue-89546.mX1f10.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/C API/2022-06-10-16-50-27.gh-issue-89546.mX1f10.rst b/Misc/NEWS.d/next/C API/2022-06-10-16-50-27.gh-issue-89546.mX1f10.rst index 1f9f9afab96552..8e6b6d95c7b4ab 100644 --- a/Misc/NEWS.d/next/C API/2022-06-10-16-50-27.gh-issue-89546.mX1f10.rst +++ b/Misc/NEWS.d/next/C API/2022-06-10-16-50-27.gh-issue-89546.mX1f10.rst @@ -1,4 +1,4 @@ -:c:func`PyType_FromMetaclass` (and other ``PyType_From*`` functions) now +:c:func:`PyType_FromMetaclass` (and other ``PyType_From*`` functions) now check that offsets and the base class's :c:member:`~PyTypeObject.tp_basicsize` fit in the new class's ``tp_basicsize``. From 5b1855201ea465dba16492f2b94029c789588a2a Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 13 Jun 2022 16:51:32 +0200 Subject: [PATCH 13/18] Downcast to int to avoid a warning --- Modules/_testcapimodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 4881b045af3fad..54957db89663e0 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -1245,7 +1245,7 @@ test_from_spec_metatype_inheritance(PyObject *self, PyObject *Py_UNUSED(ignored) goto finally; } - MinimalType_spec.basicsize = ((PyTypeObject*)class)->tp_basicsize; + MinimalType_spec.basicsize = (int)(((PyTypeObject*)class)->tp_basicsize); new = PyType_FromSpecWithBases(&MinimalType_spec, class); if (new == NULL) { goto finally; From 34edc2a43f3f9b21ffbc0c6fe23123c784054838 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 13 Jun 2022 16:51:48 +0200 Subject: [PATCH 14/18] Reflow check_basicsize_includes_size_and_offsets to conform to style guide --- Objects/typeobject.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 343b6904709dc9..c84ed555c3fd83 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3399,35 +3399,40 @@ get_bases_tuple(PyObject *bases_in, PyType_Spec *spec) } static inline int -check_basicsize_includes_size_and_offsets(PyTypeObject* type) { +check_basicsize_includes_size_and_offsets(PyTypeObject* type) +{ if (type->tp_alloc != PyType_GenericAlloc) { // Custom allocators can ignore tp_basicsize return 1; } + Py_ssize_t max = (Py_ssize_t)type->tp_basicsize; if (type->tp_base && type->tp_base->tp_basicsize > type->tp_basicsize) { PyErr_Format(PyExc_TypeError, - "tp_basicsize for type '%s' (%d) is too small for base '%s' (%d)", - type->tp_name, type->tp_basicsize, - type->tp_base->tp_name, type->tp_base->tp_basicsize); + "tp_basicsize for type '%s' (%d) is too small for base '%s' (%d)", + type->tp_name, type->tp_basicsize, + type->tp_base->tp_name, type->tp_base->tp_basicsize); return 0; } - if (type->tp_weaklistoffset + (Py_ssize_t)sizeof(PyObject*) > (Py_ssize_t)type->tp_basicsize) { + if (type->tp_weaklistoffset + (Py_ssize_t)sizeof(PyObject*) > max) { PyErr_Format(PyExc_TypeError, - "tp_basicsize for type '%s' (%d) is too small for weaklist offset %d", - type->tp_name, type->tp_basicsize, type->tp_weaklistoffset); + "tp_basicsize for type '%s' (%d) is too small for weaklist offset %d", + type->tp_name, type->tp_basicsize, + type->tp_weaklistoffset); return 0; } - if (type->tp_dictoffset + (Py_ssize_t)sizeof(PyObject*) > (Py_ssize_t)type->tp_basicsize) { + if (type->tp_dictoffset + (Py_ssize_t)sizeof(PyObject*) > max) { PyErr_Format(PyExc_TypeError, - "tp_basicsize for type '%s' (%d) is too small for dict offset %d", - type->tp_name, type->tp_basicsize, type->tp_dictoffset); + "tp_basicsize for type '%s' (%d) is too small for dict offset %d", + type->tp_name, type->tp_basicsize, + type->tp_dictoffset); return 0; } - if (type->tp_vectorcall_offset + (Py_ssize_t)sizeof(vectorcallfunc*) > (Py_ssize_t)type->tp_basicsize) { + if (type->tp_vectorcall_offset + (Py_ssize_t)sizeof(vectorcallfunc*) > max) { PyErr_Format(PyExc_TypeError, - "tp_basicsize for type '%s' (%d) is too small for vectorcall offset %d", - type->tp_name, type->tp_basicsize, type->tp_vectorcall_offset); + "tp_basicsize for type '%s' (%d) is too small for vectorcall offset %d", + type->tp_name, type->tp_basicsize, + type->tp_vectorcall_offset); return 0; } return 1; From e45f77ff5f14325715a4ed2227f4389088f482e6 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 13 Jun 2022 17:14:50 +0200 Subject: [PATCH 15/18] Reword error messages to say "offset is out of bounds" --- Objects/typeobject.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index c84ed555c3fd83..5e51f6061a5ad7 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3416,23 +3416,23 @@ check_basicsize_includes_size_and_offsets(PyTypeObject* type) } if (type->tp_weaklistoffset + (Py_ssize_t)sizeof(PyObject*) > max) { PyErr_Format(PyExc_TypeError, - "tp_basicsize for type '%s' (%d) is too small for weaklist offset %d", - type->tp_name, type->tp_basicsize, - type->tp_weaklistoffset); + "weaklist offset %d is out of bounds for type '%s' (tp_basicsize = %d)", + type->tp_weaklistoffset, + type->tp_name, type->tp_basicsize); return 0; } if (type->tp_dictoffset + (Py_ssize_t)sizeof(PyObject*) > max) { PyErr_Format(PyExc_TypeError, - "tp_basicsize for type '%s' (%d) is too small for dict offset %d", - type->tp_name, type->tp_basicsize, - type->tp_dictoffset); + "dict offset %d is out of bounds for type '%s' (tp_basicsize = %d)", + type->tp_dictoffset, + type->tp_name, type->tp_basicsize); return 0; } if (type->tp_vectorcall_offset + (Py_ssize_t)sizeof(vectorcallfunc*) > max) { PyErr_Format(PyExc_TypeError, - "tp_basicsize for type '%s' (%d) is too small for vectorcall offset %d", - type->tp_name, type->tp_basicsize, - type->tp_vectorcall_offset); + "vectorcall offset %d is out of bounds for type '%s' (tp_basicsize = %d)", + type->tp_vectorcall_offset, + type->tp_name, type->tp_basicsize); return 0; } return 1; From 9713fb4fb2f77b76485c3d0b878a3ac4ba74c204 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 13 Jun 2022 17:24:10 +0200 Subject: [PATCH 16/18] Move modname down to improve scoping --- Objects/typeobject.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 5e51f6061a5ad7..9e07da181b912c 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3447,7 +3447,6 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, * These get decrefed/freed/returned at the end, on both success and error. */ PyHeapTypeObject *res = NULL; - PyObject *modname = NULL; PyTypeObject *type; PyObject *bases = NULL; char *tp_doc = NULL; @@ -3748,12 +3747,13 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, if (r == 0) { s = strrchr(spec->name, '.'); if (s != NULL) { - modname = PyUnicode_FromStringAndSize( + PyObject *modname = PyUnicode_FromStringAndSize( spec->name, (Py_ssize_t)(s - spec->name)); if (modname == NULL) { goto finally; } r = PyDict_SetItem(type->tp_dict, &_Py_ID(__module__), modname); + Py_DECREF(modname); if (r != 0) { goto finally; } @@ -3773,7 +3773,6 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, Py_CLEAR(res); } Py_XDECREF(bases); - Py_XDECREF(modname); PyObject_Free(tp_doc); Py_XDECREF(ht_name); PyMem_Free(_ht_tpname); From a99741392696b4b24a7c5743d96c56c9a3e3d229 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 13 Jun 2022 17:32:49 +0200 Subject: [PATCH 17/18] Move variables to smaller scopes --- Objects/typeobject.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 9e07da181b912c..61d55a6db538fe 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3454,7 +3454,6 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, char *_ht_tpname = NULL; int r; - size_t len; /* Prepare slots that need special handling. * Keep in mind that a slot can be given multiple times: @@ -3465,7 +3464,6 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, Py_ssize_t nmembers = 0; Py_ssize_t weaklistoffset, dictoffset, vectorcalloffset; char *res_start; - short slot_offset, subslot_offset; nmembers = weaklistoffset = dictoffset = vectorcalloffset = 0; for (slot = spec->slots; slot->slot; slot++) { @@ -3518,7 +3516,7 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, tp_doc = NULL; } else { - len = strlen(slot->pfunc)+1; + size_t len = strlen(slot->pfunc)+1; tp_doc = PyObject_Malloc(len); if (tp_doc == NULL) { PyErr_NoMemory(); @@ -3658,7 +3656,6 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, /* Copy all the ordinary slots */ for (slot = spec->slots; slot->slot; slot++) { - PySlot_Offset slotoffsets; switch (slot->slot) { case Py_tp_base: case Py_tp_bases: @@ -3666,21 +3663,25 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, /* Processed above */ break; case Py_tp_members: - /* Move the slots to the heap type itself */ - len = Py_TYPE(type)->tp_itemsize * nmembers; - memcpy(_PyHeapType_GET_MEMBERS(res), slot->pfunc, len); - type->tp_members = _PyHeapType_GET_MEMBERS(res); + { + /* Move the slots to the heap type itself */ + size_t len = Py_TYPE(type)->tp_itemsize * nmembers; + memcpy(_PyHeapType_GET_MEMBERS(res), slot->pfunc, len); + type->tp_members = _PyHeapType_GET_MEMBERS(res); + } break; default: - /* Copy other slots directly */ - slotoffsets = pyslot_offsets[slot->slot]; - slot_offset = slotoffsets.slot_offset; - if (slotoffsets.subslot_offset == -1) { - *(void**)((char*)res_start + slot_offset) = slot->pfunc; - } else { - void *parent_slot = *(void**)((char*)res_start + slot_offset); - subslot_offset = slotoffsets.subslot_offset; - *(void**)((char*)parent_slot + subslot_offset) = slot->pfunc; + { + /* Copy other slots directly */ + PySlot_Offset slotoffsets = pyslot_offsets[slot->slot]; + short slot_offset = slotoffsets.slot_offset; + if (slotoffsets.subslot_offset == -1) { + *(void**)((char*)res_start + slot_offset) = slot->pfunc; + } else { + void *procs = *(void**)((char*)res_start + slot_offset); + short subslot_offset = slotoffsets.subslot_offset; + *(void**)((char*)procs + subslot_offset) = slot->pfunc; + } } break; } From f53d86c7bc48d568ba5850e0bb34ad64838bbeb4 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 14 Jun 2022 10:49:43 +0200 Subject: [PATCH 18/18] One more PEP 7 nit! Co-authored-by: Erlend Egeberg Aasland --- Objects/typeobject.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 001c9e1b82a387..db4682c69ed0e3 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3677,7 +3677,8 @@ PyType_FromMetaclass(PyTypeObject *metaclass, PyObject *module, short slot_offset = slotoffsets.slot_offset; if (slotoffsets.subslot_offset == -1) { *(void**)((char*)res_start + slot_offset) = slot->pfunc; - } else { + } + else { void *procs = *(void**)((char*)res_start + slot_offset); short subslot_offset = slotoffsets.subslot_offset; *(void**)((char*)procs + subslot_offset) = slot->pfunc;