From 3a9cc2120fbcc73c9d70bc3205201c8845307ff2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 25 Jan 2025 16:00:18 +0100 Subject: [PATCH 1/8] fix UBSan failures for `PyStructObject` --- Modules/_struct.c | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/Modules/_struct.c b/Modules/_struct.c index 21582b945be23d..0a261d40921e42 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -73,7 +73,8 @@ typedef struct { PyObject *weakreflist; /* List of weak references */ } PyStructObject; -#define PyStruct_Check(op, state) PyObject_TypeCheck(op, (PyTypeObject *)(state)->PyStructType) +#define _PyStructObject_CAST(op) ((PyStructObject *)(op)) +#define PyStruct_Check(op, state) PyObject_TypeCheck(op, (PyTypeObject *)(state)->PyStructType) /* Define various structs to figure out the alignments of types */ @@ -1853,33 +1854,36 @@ Struct___init___impl(PyStructObject *self, PyObject *format) } static int -s_clear(PyStructObject *s) +s_clear(PyObject *op) { + PyStructObject *s = _PyStructObject_CAST(op); Py_CLEAR(s->s_format); return 0; } static int -s_traverse(PyStructObject *s, visitproc visit, void *arg) +s_traverse(PyObject *op, visitproc visit, void *arg) { + PyStructObject *s = _PyStructObject_CAST(op); Py_VISIT(Py_TYPE(s)); Py_VISIT(s->s_format); return 0; } static void -s_dealloc(PyStructObject *s) +s_dealloc(PyObject *op) { + PyStructObject *s = _PyStructObject_CAST(op); PyTypeObject *tp = Py_TYPE(s); PyObject_GC_UnTrack(s); - if (s->weakreflist != NULL) - PyObject_ClearWeakRefs((PyObject *)s); + if (s->weakreflist != NULL) { + PyObject_ClearWeakRefs(op); + } if (s->s_codes != NULL) { PyMem_Free(s->s_codes); } Py_XDECREF(s->s_format); - freefunc free_func = PyType_GetSlot(Py_TYPE(s), Py_tp_free); - free_func(s); + tp->tp_free(s); Py_DECREF(tp); } @@ -2264,7 +2268,7 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs) _structmodulestate *state = get_struct_state_structinst(self); /* Validate arguments. */ - soself = (PyStructObject *)self; + soself = _PyStructObject_CAST(self); assert(PyStruct_Check(self, state)); assert(soself->s_codes != NULL); if (nargs != soself->s_len) @@ -2309,7 +2313,7 @@ s_pack_into(PyObject *self, PyObject *const *args, Py_ssize_t nargs) _structmodulestate *state = get_struct_state_structinst(self); /* Validate arguments. +1 is for the first arg as buffer. */ - soself = (PyStructObject *)self; + soself = _PyStructObject_CAST(self); assert(PyStruct_Check(self, state)); assert(soself->s_codes != NULL); if (nargs != (soself->s_len + 2)) @@ -2395,15 +2399,17 @@ s_pack_into(PyObject *self, PyObject *const *args, Py_ssize_t nargs) } static PyObject * -s_get_format(PyStructObject *self, void *unused) +s_get_format(PyObject *op, void *Py_UNUSED(closure)) { + PyStructObject *self = _PyStructObject_CAST(op); return PyUnicode_FromStringAndSize(PyBytes_AS_STRING(self->s_format), PyBytes_GET_SIZE(self->s_format)); } static PyObject * -s_get_size(PyStructObject *self, void *unused) +s_get_size(PyObject *op, void *Py_UNUSED(closure)) { + PyStructObject *self = _PyStructObject_CAST(op); return PyLong_FromSsize_t(self->s_size); } @@ -2411,8 +2417,9 @@ PyDoc_STRVAR(s_sizeof__doc__, "S.__sizeof__() -> size of S in memory, in bytes"); static PyObject * -s_sizeof(PyStructObject *self, void *unused) +s_sizeof(PyObject *op, PyObject *Py_UNUSED(unused)) { + PyStructObject *self = _PyStructObject_CAST(op); size_t size = _PyObject_SIZE(Py_TYPE(self)) + sizeof(formatcode); for (formatcode *code = self->s_codes; code->fmtdef != NULL; code++) { size += sizeof(formatcode); @@ -2421,8 +2428,9 @@ s_sizeof(PyStructObject *self, void *unused) } static PyObject * -s_repr(PyStructObject *self) +s_repr(PyObject *op) { + PyStructObject *self = _PyStructObject_CAST(op); PyObject* fmt = PyUnicode_FromStringAndSize( PyBytes_AS_STRING(self->s_format), PyBytes_GET_SIZE(self->s_format)); if (fmt == NULL) { @@ -2441,7 +2449,7 @@ static struct PyMethodDef s_methods[] = { {"pack_into", _PyCFunction_CAST(s_pack_into), METH_FASTCALL, s_pack_into__doc__}, STRUCT_UNPACK_METHODDEF STRUCT_UNPACK_FROM_METHODDEF - {"__sizeof__", (PyCFunction)s_sizeof, METH_NOARGS, s_sizeof__doc__}, + {"__sizeof__", s_sizeof, METH_NOARGS, s_sizeof__doc__}, {NULL, NULL} /* sentinel */ }; @@ -2451,8 +2459,8 @@ static PyMemberDef s_members[] = { }; static PyGetSetDef s_getsetlist[] = { - {"format", (getter)s_get_format, (setter)NULL, PyDoc_STR("struct format string"), NULL}, - {"size", (getter)s_get_size, (setter)NULL, PyDoc_STR("struct size in bytes"), NULL}, + {"format", s_get_format, NULL, PyDoc_STR("struct format string"), NULL}, + {"size", s_get_size, NULL, PyDoc_STR("struct size in bytes"), NULL}, {NULL} /* sentinel */ }; @@ -2508,7 +2516,7 @@ cache_struct_converter(PyObject *module, PyObject *fmt, PyStructObject **ptr) return 0; } if (s_object != NULL) { - *ptr = (PyStructObject *)s_object; + *ptr = _PyStructObject_CAST(s_object); return Py_CLEANUP_SUPPORTED; } From 56130791b2a4b19165d4e6d431409a71c3bb8e3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 25 Jan 2025 16:00:26 +0100 Subject: [PATCH 2/8] suppress unused return values --- Modules/_struct.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_struct.c b/Modules/_struct.c index 0a261d40921e42..135a168bf00572 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -2759,7 +2759,7 @@ _structmodule_clear(PyObject *module) static void _structmodule_free(void *module) { - _structmodule_clear((PyObject *)module); + (void)_structmodule_clear((PyObject *)module); } static int From 69feffd892e5f2f5fc46b7d31735ccfea3dd775f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 25 Jan 2025 16:55:46 +0100 Subject: [PATCH 3/8] fix UBSan failures for `unpackiterobject` --- Modules/_struct.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/Modules/_struct.c b/Modules/_struct.c index 135a168bf00572..544fed6fcdb7cf 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -2030,9 +2030,12 @@ typedef struct { Py_ssize_t index; } unpackiterobject; +#define _unpackiterobject_CAST(op) ((unpackiterobject *)(op)) + static void -unpackiter_dealloc(unpackiterobject *self) +unpackiter_dealloc(PyObject *op) { + unpackiterobject *self = _unpackiterobject_CAST(op); /* bpo-31095: UnTrack is needed before calling any callbacks */ PyTypeObject *tp = Py_TYPE(self); PyObject_GC_UnTrack(self); @@ -2043,8 +2046,9 @@ unpackiter_dealloc(unpackiterobject *self) } static int -unpackiter_traverse(unpackiterobject *self, visitproc visit, void *arg) +unpackiter_traverse(PyObject *op, visitproc visit, void *arg) { + unpackiterobject *self = _unpackiterobject_CAST(op); Py_VISIT(Py_TYPE(self)); Py_VISIT(self->so); Py_VISIT(self->buf.obj); @@ -2052,28 +2056,33 @@ unpackiter_traverse(unpackiterobject *self, visitproc visit, void *arg) } static PyObject * -unpackiter_len(unpackiterobject *self, PyObject *Py_UNUSED(ignored)) +unpackiter_len(PyObject *op, PyObject *Py_UNUSED(args)) { Py_ssize_t len; - if (self->so == NULL) + unpackiterobject *self = _unpackiterobject_CAST(op); + if (self->so == NULL) { len = 0; - else + } + else { len = (self->buf.len - self->index) / self->so->s_size; + } return PyLong_FromSsize_t(len); } static PyMethodDef unpackiter_methods[] = { - {"__length_hint__", (PyCFunction) unpackiter_len, METH_NOARGS, NULL}, + {"__length_hint__", unpackiter_len, METH_NOARGS, NULL}, {NULL, NULL} /* sentinel */ }; static PyObject * -unpackiter_iternext(unpackiterobject *self) +unpackiter_iternext(PyObject *op) { + unpackiterobject *self = _unpackiterobject_CAST(op); _structmodulestate *state = get_struct_state_iterinst(self); PyObject *result; - if (self->so == NULL) + if (self->so == NULL) { return NULL; + } if (self->index >= self->buf.len) { /* Iterator exhausted */ Py_CLEAR(self->so); @@ -2082,7 +2091,7 @@ unpackiter_iternext(unpackiterobject *self) } assert(self->index + self->so->s_size <= self->buf.len); result = s_unpack_internal(self->so, - (char*) self->buf.buf + self->index, + (char *)self->buf.buf + self->index, state); self->index += self->so->s_size; return result; From 0e700b0b070593b1294d096f5c651bc6f1231027 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 8 Feb 2025 10:16:29 +0100 Subject: [PATCH 4/8] Do Do not use `_` + capital letter in new code as it is also UB. --- Modules/_struct.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/Modules/_struct.c b/Modules/_struct.c index 544fed6fcdb7cf..b81a160b8b1ae9 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -73,7 +73,7 @@ typedef struct { PyObject *weakreflist; /* List of weak references */ } PyStructObject; -#define _PyStructObject_CAST(op) ((PyStructObject *)(op)) +#define PyStructObject_CAST(op) ((PyStructObject *)(op)) #define PyStruct_Check(op, state) PyObject_TypeCheck(op, (PyTypeObject *)(state)->PyStructType) /* Define various structs to figure out the alignments of types */ @@ -1856,7 +1856,7 @@ Struct___init___impl(PyStructObject *self, PyObject *format) static int s_clear(PyObject *op) { - PyStructObject *s = _PyStructObject_CAST(op); + PyStructObject *s = PyStructObject_CAST(op); Py_CLEAR(s->s_format); return 0; } @@ -1864,7 +1864,7 @@ s_clear(PyObject *op) static int s_traverse(PyObject *op, visitproc visit, void *arg) { - PyStructObject *s = _PyStructObject_CAST(op); + PyStructObject *s = PyStructObject_CAST(op); Py_VISIT(Py_TYPE(s)); Py_VISIT(s->s_format); return 0; @@ -1873,7 +1873,7 @@ s_traverse(PyObject *op, visitproc visit, void *arg) static void s_dealloc(PyObject *op) { - PyStructObject *s = _PyStructObject_CAST(op); + PyStructObject *s = PyStructObject_CAST(op); PyTypeObject *tp = Py_TYPE(s); PyObject_GC_UnTrack(s); if (s->weakreflist != NULL) { @@ -2277,7 +2277,7 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs) _structmodulestate *state = get_struct_state_structinst(self); /* Validate arguments. */ - soself = _PyStructObject_CAST(self); + soself = PyStructObject_CAST(self); assert(PyStruct_Check(self, state)); assert(soself->s_codes != NULL); if (nargs != soself->s_len) @@ -2322,7 +2322,7 @@ s_pack_into(PyObject *self, PyObject *const *args, Py_ssize_t nargs) _structmodulestate *state = get_struct_state_structinst(self); /* Validate arguments. +1 is for the first arg as buffer. */ - soself = _PyStructObject_CAST(self); + soself = PyStructObject_CAST(self); assert(PyStruct_Check(self, state)); assert(soself->s_codes != NULL); if (nargs != (soself->s_len + 2)) @@ -2410,7 +2410,7 @@ s_pack_into(PyObject *self, PyObject *const *args, Py_ssize_t nargs) static PyObject * s_get_format(PyObject *op, void *Py_UNUSED(closure)) { - PyStructObject *self = _PyStructObject_CAST(op); + PyStructObject *self = PyStructObject_CAST(op); return PyUnicode_FromStringAndSize(PyBytes_AS_STRING(self->s_format), PyBytes_GET_SIZE(self->s_format)); } @@ -2418,7 +2418,7 @@ s_get_format(PyObject *op, void *Py_UNUSED(closure)) static PyObject * s_get_size(PyObject *op, void *Py_UNUSED(closure)) { - PyStructObject *self = _PyStructObject_CAST(op); + PyStructObject *self = PyStructObject_CAST(op); return PyLong_FromSsize_t(self->s_size); } @@ -2428,7 +2428,7 @@ PyDoc_STRVAR(s_sizeof__doc__, static PyObject * s_sizeof(PyObject *op, PyObject *Py_UNUSED(unused)) { - PyStructObject *self = _PyStructObject_CAST(op); + PyStructObject *self = PyStructObject_CAST(op); size_t size = _PyObject_SIZE(Py_TYPE(self)) + sizeof(formatcode); for (formatcode *code = self->s_codes; code->fmtdef != NULL; code++) { size += sizeof(formatcode); @@ -2439,7 +2439,7 @@ s_sizeof(PyObject *op, PyObject *Py_UNUSED(unused)) static PyObject * s_repr(PyObject *op) { - PyStructObject *self = _PyStructObject_CAST(op); + PyStructObject *self = PyStructObject_CAST(op); PyObject* fmt = PyUnicode_FromStringAndSize( PyBytes_AS_STRING(self->s_format), PyBytes_GET_SIZE(self->s_format)); if (fmt == NULL) { @@ -2525,7 +2525,7 @@ cache_struct_converter(PyObject *module, PyObject *fmt, PyStructObject **ptr) return 0; } if (s_object != NULL) { - *ptr = _PyStructObject_CAST(s_object); + *ptr = PyStructObject_CAST(s_object); return Py_CLEANUP_SUPPORTED; } From 3483d82047f17cd31645ec36e0f1b3deb543bfbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 8 Feb 2025 10:16:43 +0100 Subject: [PATCH 5/8] Do not add an underscore to the macro if none is needed. --- Modules/_struct.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Modules/_struct.c b/Modules/_struct.c index b81a160b8b1ae9..dcfcce8700e140 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -2030,12 +2030,12 @@ typedef struct { Py_ssize_t index; } unpackiterobject; -#define _unpackiterobject_CAST(op) ((unpackiterobject *)(op)) +#define unpackiterobject_CAST(op) ((unpackiterobject *)(op)) static void unpackiter_dealloc(PyObject *op) { - unpackiterobject *self = _unpackiterobject_CAST(op); + unpackiterobject *self = unpackiterobject_CAST(op); /* bpo-31095: UnTrack is needed before calling any callbacks */ PyTypeObject *tp = Py_TYPE(self); PyObject_GC_UnTrack(self); @@ -2048,7 +2048,7 @@ unpackiter_dealloc(PyObject *op) static int unpackiter_traverse(PyObject *op, visitproc visit, void *arg) { - unpackiterobject *self = _unpackiterobject_CAST(op); + unpackiterobject *self = unpackiterobject_CAST(op); Py_VISIT(Py_TYPE(self)); Py_VISIT(self->so); Py_VISIT(self->buf.obj); @@ -2059,7 +2059,7 @@ static PyObject * unpackiter_len(PyObject *op, PyObject *Py_UNUSED(args)) { Py_ssize_t len; - unpackiterobject *self = _unpackiterobject_CAST(op); + unpackiterobject *self = unpackiterobject_CAST(op); if (self->so == NULL) { len = 0; } @@ -2077,7 +2077,7 @@ static PyMethodDef unpackiter_methods[] = { static PyObject * unpackiter_iternext(PyObject *op) { - unpackiterobject *self = _unpackiterobject_CAST(op); + unpackiterobject *self = unpackiterobject_CAST(op); _structmodulestate *state = get_struct_state_iterinst(self); PyObject *result; if (self->so == NULL) { From 5c360bdfb46ef579f14c785be4b4c36e71549f54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 22 Feb 2025 10:50:11 +0100 Subject: [PATCH 6/8] fix semantic naming --- Modules/_struct.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_struct.c b/Modules/_struct.c index dcfcce8700e140..a5036e077d1dd6 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -2056,7 +2056,7 @@ unpackiter_traverse(PyObject *op, visitproc visit, void *arg) } static PyObject * -unpackiter_len(PyObject *op, PyObject *Py_UNUSED(args)) +unpackiter_len(PyObject *op, PyObject *Py_UNUSED(dummy)) { Py_ssize_t len; unpackiterobject *self = unpackiterobject_CAST(op); From 27f6364845c0bb6c295090acf6c917aed666c9a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sun, 23 Feb 2025 11:10:01 +0100 Subject: [PATCH 7/8] use better semantic naming --- Modules/_struct.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_struct.c b/Modules/_struct.c index a5036e077d1dd6..fcd290374f3bf2 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -2426,7 +2426,7 @@ PyDoc_STRVAR(s_sizeof__doc__, "S.__sizeof__() -> size of S in memory, in bytes"); static PyObject * -s_sizeof(PyObject *op, PyObject *Py_UNUSED(unused)) +s_sizeof(PyObject *op, PyObject *Py_UNUSED(dummy)) { PyStructObject *self = PyStructObject_CAST(op); size_t size = _PyObject_SIZE(Py_TYPE(self)) + sizeof(formatcode); From 9f13d2817cdf1258a0be7c1613c84171289a0ba1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sun, 23 Feb 2025 11:11:13 +0100 Subject: [PATCH 8/8] revert a space that increased the diff and was more inconsistent --- Modules/_struct.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_struct.c b/Modules/_struct.c index fcd290374f3bf2..f096998cb714e7 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -2091,7 +2091,7 @@ unpackiter_iternext(PyObject *op) } assert(self->index + self->so->s_size <= self->buf.len); result = s_unpack_internal(self->so, - (char *)self->buf.buf + self->index, + (char*)self->buf.buf + self->index, state); self->index += self->so->s_size; return result;