From ac4e5bb97625c4f3c545911de31de19baa36caed Mon Sep 17 00:00:00 2001 From: Eric Wieser Date: Mon, 5 Feb 2018 21:28:18 -0800 Subject: [PATCH 1/7] bpo-32780: Fix the PEP3118 format string for ctypes.Structure Without this fix, it would never include padding bytes - an assumption that was only valid in the case when `_pack_` was set - and this case was explicitly not implemented. This should allow conversion from ctypes structs to numpy structs. --- Lib/ctypes/test/test_pep3118.py | 23 ++- .../2018-02-05-21-54-46.bpo-32780.Dtiz8z.rst | 3 + Modules/_ctypes/stgdict.c | 131 ++++++++++++++---- 3 files changed, 127 insertions(+), 30 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2018-02-05-21-54-46.bpo-32780.Dtiz8z.rst diff --git a/Lib/ctypes/test/test_pep3118.py b/Lib/ctypes/test/test_pep3118.py index 81e8ca7638fdeb..8486ad705c206b 100644 --- a/Lib/ctypes/test/test_pep3118.py +++ b/Lib/ctypes/test/test_pep3118.py @@ -86,6 +86,20 @@ class PackedPoint(Structure): _pack_ = 2 _fields_ = [("x", c_long), ("y", c_long)] +class PointMidPad(Structure): + _fields_ = [("x", c_byte), ("y", c_uint64)] + +class PackedPointMidPad(Structure): + _pack_ = 2 + _fields_ = [("x", c_byte), ("y", c_uint64)] + +class PointEndPad(Structure): + _fields_ = [("x", c_uint64), ("y", c_byte)] + +class PackedPointEndPad(Structure): + _pack_ = 2 + _fields_ = [("x", c_uint64), ("y", c_byte)] + class Point2(Structure): pass Point2._fields_ = [("x", c_long), ("y", c_long)] @@ -183,10 +197,13 @@ class Complete(Structure): ## structures and unions - (Point, "T{ 0) { + log_n++; + n /= 10; + } + return log_n; +} + +/* + Append {padding}x to the PEP3118 format string. +*/ +char * +_ctypes_alloc_format_padding(const char *prefix, Py_ssize_t padding) +{ + char *result; + char *buf; + + assert(padding > 0); + + if (padding == 1) { + /* Use x instead of 1x, for brevity */ + return _ctypes_alloc_format_string(prefix, "x"); + } + + /* decimal characters + x + null */ + buf = PyMem_Malloc(clog10(padding) + 2); + if (buf == NULL) { + PyErr_NoMemory(); + return NULL; + } + sprintf(buf, "%zdx", padding); + result = _ctypes_alloc_format_string(prefix, buf); + PyMem_Free(buf); + return result; +} + /* Retrieve the (optional) _pack_ attribute from a type, the _fields_ attribute, and create an StgDictObject. Used for Structure and Union subclasses. @@ -337,7 +379,7 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct { StgDictObject *stgdict, *basedict; Py_ssize_t len, offset, size, align, i; - Py_ssize_t union_size, total_align; + Py_ssize_t union_size, total_align, aligned_size; Py_ssize_t field_size = 0; int bitofs; PyObject *isPacked; @@ -443,12 +485,10 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct } assert(stgdict->format == NULL); - if (isStruct && !isPacked) { + if (isStruct) { stgdict->format = _ctypes_alloc_format_string(NULL, "T{"); } else { - /* PEP3118 doesn't support union, or packed structures (well, - only standard packing, but we don't support the pep for - that). Use 'B' for bytes. */ + /* PEP3118 doesn't support union. Use 'B' for bytes. */ stgdict->format = _ctypes_alloc_format_string(NULL, "B"); } if (stgdict->format == NULL) @@ -515,12 +555,14 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct } else bitsize = 0; - if (isStruct && !isPacked) { + if (isStruct) { const char *fieldfmt = dict->format ? dict->format : "B"; const char *fieldname = PyUnicode_AsUTF8(name); char *ptr; Py_ssize_t len; char *buf; + Py_ssize_t last_size = size; + Py_ssize_t padding; if (fieldname == NULL) { @@ -528,11 +570,36 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct return -1; } + prop = PyCField_FromDesc(desc, i, + &field_size, bitsize, &bitofs, + &size, &offset, &align, + pack, big_endian); + if (prop == NULL) { + Py_DECREF(pair); + return -1; + } + + /* number of bytes between the end of the last field and the start + of this one */ + padding = ((CFieldObject *)prop)->offset - last_size; + + if (padding > 0) { + ptr = stgdict->format; + stgdict->format = _ctypes_alloc_format_padding(ptr, padding); + PyMem_Free(ptr); + if (stgdict->format == NULL) { + Py_DECREF(pair); + Py_DECREF(prop); + return -1; + } + } + len = strlen(fieldname) + strlen(fieldfmt); buf = PyMem_Malloc(len + 2 + 1); if (buf == NULL) { Py_DECREF(pair); + Py_DECREF(prop); PyErr_NoMemory(); return -1; } @@ -550,15 +617,9 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct if (stgdict->format == NULL) { Py_DECREF(pair); + Py_DECREF(prop); return -1; } - } - - if (isStruct) { - prop = PyCField_FromDesc(desc, i, - &field_size, bitsize, &bitofs, - &size, &offset, &align, - pack, big_endian); } else /* union */ { size = 0; offset = 0; @@ -567,14 +628,14 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct &field_size, bitsize, &bitofs, &size, &offset, &align, pack, big_endian); + if (prop == NULL) { + Py_DECREF(pair); + return -1; + } union_size = max(size, union_size); } total_align = max(align, total_align); - if (!prop) { - Py_DECREF(pair); - return -1; - } if (-1 == PyObject_SetAttr(type, name, prop)) { Py_DECREF(prop); Py_DECREF(pair); @@ -585,29 +646,45 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct } #undef realdict - if (isStruct && !isPacked) { - char *ptr = stgdict->format; + if (!isStruct) { + size = union_size; + } + + /* Adjust the size according to the alignment requirements */ + aligned_size = ((size + total_align - 1) / total_align) * total_align; + + if (isStruct) { + char *ptr; + Py_ssize_t padding; + + /* Pad up to the full size of the struct */ + padding = aligned_size - size; + if (padding > 0) { + ptr = stgdict->format; + stgdict->format = _ctypes_alloc_format_padding(ptr, padding); + PyMem_Free(ptr); + if (stgdict->format == NULL) { + return -1; + } + } + + ptr = stgdict->format; stgdict->format = _ctypes_alloc_format_string(stgdict->format, "}"); PyMem_Free(ptr); if (stgdict->format == NULL) return -1; } - if (!isStruct) - size = union_size; - - /* Adjust the size according to the alignment requirements */ - size = ((size + total_align - 1) / total_align) * total_align; - stgdict->ffi_type_pointer.alignment = Py_SAFE_DOWNCAST(total_align, Py_ssize_t, unsigned short); - stgdict->ffi_type_pointer.size = size; + stgdict->ffi_type_pointer.size = aligned_size; - stgdict->size = size; + stgdict->size = aligned_size; stgdict->align = total_align; stgdict->length = len; /* ADD ffi_ofs? */ + /* We did check that this flag was NOT set above, it must not have been set until now. */ if (stgdict->flags & DICTFLAG_FINAL) { From 6bf5a25bcb3ac61938570aaf45b698ef28fea4d4 Mon Sep 17 00:00:00 2001 From: Eric Wieser Date: Fri, 8 Jul 2022 09:31:09 +0000 Subject: [PATCH 2/7] remove unused variable, extend documentation --- Doc/library/ctypes.rst | 1 + Modules/_ctypes/stgdict.c | 4 +--- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/Doc/library/ctypes.rst b/Doc/library/ctypes.rst index 475737144d40c6..d698806b2f3e46 100644 --- a/Doc/library/ctypes.rst +++ b/Doc/library/ctypes.rst @@ -2481,6 +2481,7 @@ fields, or any other data types containing pointer type fields. An optional small integer that allows overriding the alignment of structure fields in the instance. :attr:`_pack_` must already be defined when :attr:`_fields_` is assigned, otherwise it will have no effect. + Setting this attribute to 0 is the same as not setting it at all. .. attribute:: _anonymous_ diff --git a/Modules/_ctypes/stgdict.c b/Modules/_ctypes/stgdict.c index bde3e0d76a37df..fc3564bb4ea3a0 100644 --- a/Modules/_ctypes/stgdict.c +++ b/Modules/_ctypes/stgdict.c @@ -398,7 +398,6 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct Py_ssize_t field_size = 0; int bitofs; PyObject *tmp; - int isPacked; int pack; Py_ssize_t ffi_ofs; int big_endian; @@ -443,7 +442,6 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct return -1; } if (tmp) { - isPacked = 1; pack = _PyLong_AsInt(tmp); Py_DECREF(tmp); if (pack < 0) { @@ -458,7 +456,7 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct } } else { - isPacked = 0; + /* Setting `_pack_ = 0` amounts to using the default alignment */ pack = 0; } From 3b7f14fb2530cf569187d882eb24255e3ba56189 Mon Sep 17 00:00:00 2001 From: Eric Wieser Date: Fri, 8 Jul 2022 09:40:19 +0000 Subject: [PATCH 3/7] comment --- Modules/_ctypes/stgdict.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Modules/_ctypes/stgdict.c b/Modules/_ctypes/stgdict.c index fc3564bb4ea3a0..ffc1f4d90b0210 100644 --- a/Modules/_ctypes/stgdict.c +++ b/Modules/_ctypes/stgdict.c @@ -620,6 +620,8 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct return -1; } + /* construct the field now, as `prop->offset` is `offset` with + corrected alignment */ prop = PyCField_FromDesc(desc, i, &field_size, bitsize, &bitofs, &size, &offset, &align, From 27b1601038a87f76f32bffbabf1877ff9345a520 Mon Sep 17 00:00:00 2001 From: Eric Wieser Date: Mon, 15 Aug 2022 10:55:03 +0000 Subject: [PATCH 4/7] fix incorrect name and documentation --- Modules/_ctypes/stgdict.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Modules/_ctypes/stgdict.c b/Modules/_ctypes/stgdict.c index ffc1f4d90b0210..b73395ac0ee246 100644 --- a/Modules/_ctypes/stgdict.c +++ b/Modules/_ctypes/stgdict.c @@ -341,10 +341,10 @@ MakeAnonFields(PyObject *type) } /* - Compute ceil(log10(x)), for the purpose of determining string lengths. + Compute `floor(log10(x)) + 1`, for the purpose of determining string lengths. */ static Py_ssize_t -clog10(Py_ssize_t n) +num_digits_of(Py_ssize_t n) { Py_ssize_t log_n = 0; while (n > 0) { @@ -371,7 +371,7 @@ _ctypes_alloc_format_padding(const char *prefix, Py_ssize_t padding) } /* decimal characters + x + null */ - buf = PyMem_Malloc(clog10(padding) + 2); + buf = PyMem_Malloc(num_digits_of(padding) + 2); if (buf == NULL) { PyErr_NoMemory(); return NULL; From e300e164394695c5219189fb178cb6849de4754f Mon Sep 17 00:00:00 2001 From: Eric Wieser Date: Mon, 15 Aug 2022 10:55:40 +0000 Subject: [PATCH 5/7] remove the heap allocation as requested --- Modules/_ctypes/stgdict.c | 28 +++------------------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/Modules/_ctypes/stgdict.c b/Modules/_ctypes/stgdict.c index b73395ac0ee246..217033669327dd 100644 --- a/Modules/_ctypes/stgdict.c +++ b/Modules/_ctypes/stgdict.c @@ -340,28 +340,14 @@ MakeAnonFields(PyObject *type) return 0; } -/* - Compute `floor(log10(x)) + 1`, for the purpose of determining string lengths. -*/ -static Py_ssize_t -num_digits_of(Py_ssize_t n) -{ - Py_ssize_t log_n = 0; - while (n > 0) { - log_n++; - n /= 10; - } - return log_n; -} - /* Append {padding}x to the PEP3118 format string. */ char * _ctypes_alloc_format_padding(const char *prefix, Py_ssize_t padding) { - char *result; - char *buf; + /* int64 decimal characters + x + null */ + char buf[19 + 1 + 1]; assert(padding > 0); @@ -370,16 +356,8 @@ _ctypes_alloc_format_padding(const char *prefix, Py_ssize_t padding) return _ctypes_alloc_format_string(prefix, "x"); } - /* decimal characters + x + null */ - buf = PyMem_Malloc(num_digits_of(padding) + 2); - if (buf == NULL) { - PyErr_NoMemory(); - return NULL; - } sprintf(buf, "%zdx", padding); - result = _ctypes_alloc_format_string(prefix, buf); - PyMem_Free(buf); - return result; + return _ctypes_alloc_format_string(prefix, buf); } /* From 4de7a8a2917e8370d2a0a16beeac178a66e74e63 Mon Sep 17 00:00:00 2001 From: Eric Wieser Date: Sun, 5 Feb 2023 12:37:21 +0000 Subject: [PATCH 6/7] Update Misc/NEWS.d/next/Core and Builtins/2018-02-05-21-54-46.bpo-32780.Dtiz8z.rst Co-authored-by: Mark Dickinson --- .../Core and Builtins/2018-02-05-21-54-46.bpo-32780.Dtiz8z.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-02-05-21-54-46.bpo-32780.Dtiz8z.rst b/Misc/NEWS.d/next/Core and Builtins/2018-02-05-21-54-46.bpo-32780.Dtiz8z.rst index e7a85b0033ddff..8996d471159a64 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2018-02-05-21-54-46.bpo-32780.Dtiz8z.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2018-02-05-21-54-46.bpo-32780.Dtiz8z.rst @@ -1,3 +1,3 @@ Inter-field padding is now inserted into the PEP3118 format strings obtained -from ``ctypes.Structure`` objects, reflecting their true representation in +from :class:`ctypes.Structure` objects, reflecting their true representation in memory. From 26311aecd38ae115f88b38708bc65eacbbcab651 Mon Sep 17 00:00:00 2001 From: Eric Wieser Date: Sun, 5 Feb 2023 14:26:13 +0000 Subject: [PATCH 7/7] Apply suggestions from code review --- Modules/_ctypes/stgdict.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Modules/_ctypes/stgdict.c b/Modules/_ctypes/stgdict.c index d4710854c3bd3c..dac772e3073112 100644 --- a/Modules/_ctypes/stgdict.c +++ b/Modules/_ctypes/stgdict.c @@ -338,7 +338,9 @@ MakeAnonFields(PyObject *type) } /* - Append {padding}x to the PEP3118 format string. + Allocate a memory block for a pep3118 format string, copy prefix (if + non-null) into it and append `{padding}x` to the end. + Returns NULL on failure, with the error indicator set. */ char * _ctypes_alloc_format_padding(const char *prefix, Py_ssize_t padding) @@ -353,7 +355,8 @@ _ctypes_alloc_format_padding(const char *prefix, Py_ssize_t padding) return _ctypes_alloc_format_string(prefix, "x"); } - sprintf(buf, "%zdx", padding); + int ret = PyOS_snprintf(buf, sizeof(buf), "%zdx", padding); + assert(0 <= ret && ret < sizeof(buf)); return _ctypes_alloc_format_string(prefix, buf); }