[Python-checkins] bpo-29753: fix merging packed bitfields in ctypes struct/union (GH-19850)

[miss-islington]

https://github.com/python/cpython/commit/0d7ad9fb38c041c46094087b0cf2c8ce44916b11
commit: 0d7ad9fb38c041c46094087b0cf2c8ce44916b11
branch: master
author: Filipe Laíns <lains at riseup.net>
committer: miss-islington <31488909+miss-islington at users.noreply.github.com>
date: 2021-02-28T14:43:19-08:00
summary:

bpo-29753: fix merging packed bitfields in ctypes struct/union (GH-19850)



>From the commit message:

> When the structure is packed we should always expand when needed,
> otherwise we will add some padding between the fields. This patch makes
> sure we always merge bitfields together. It also changes the field merging
> algorithm so that it handles bitfields correctly.

Automerge-Triggered-By: GH:jaraco

files:
A Misc/NEWS.d/next/Library/2020-05-02-01-01-30.bpo-29753.n2M-AF.rst
M Lib/ctypes/test/test_bitfields.py
M Modules/_ctypes/cfield.c

diff --git a/Lib/ctypes/test/test_bitfields.py b/Lib/ctypes/test/test_bitfields.py
index 992b8c4da3a77..68b618a05f436 100644
--- a/Lib/ctypes/test/test_bitfields.py
+++ b/Lib/ctypes/test/test_bitfields.py
@@ -233,6 +233,69 @@ class X(Structure):
         else:
             self.assertEqual(sizeof(X), sizeof(c_int) * 2)
 
+    @unittest.skipIf(os.name == 'nt', reason='Posix only')
+    def test_packed_posix(self):
+        test_cases = {
+            (
+                ("a", c_uint8, 4),
+                ("b", c_uint8, 4),
+            ): 1,
+            (
+                ("a", c_uint8, 1),
+                ("b", c_uint16, 1),
+                ("c", c_uint32, 1),
+                ("d", c_uint64, 1),
+            ): 1,
+            (
+                ("a", c_uint8, 8),
+                ("b", c_uint16, 1),
+                ("c", c_uint32, 1),
+                ("d", c_uint64, 1),
+            ): 2,
+            (
+                ("a", c_uint32, 9),
+                ("b", c_uint16, 10),
+                ("c", c_uint32, 25),
+                ("d", c_uint64, 1),
+            ): 6,
+            (
+                ("a", c_uint32, 9),
+                ("b", c_uint16, 10),
+                ("c", c_uint32, 25),
+                ("d", c_uint64, 5),
+            ): 7,
+            (
+                ("a", c_uint16),
+                ("b", c_uint16, 9),
+                ("c", c_uint16, 1),
+                ("d", c_uint16, 1),
+                ("e", c_uint16, 1),
+                ("f", c_uint16, 1),
+                ("g", c_uint16, 3),
+                ("h", c_uint32, 10),
+                ("i", c_uint32, 20),
+                ("j", c_uint32, 2),
+            ): 8,
+            (
+                ("a", c_uint16, 9),
+                ("b", c_uint16, 10),
+                ("d", c_uint16),
+                ("c", c_uint8, 8),
+            ): 6,
+            (
+                ("a", c_uint32, 9),
+                ("b", c_uint32),
+                ("c", c_uint32, 8),
+            ): 7,
+        }
+
+        for fields, size in test_cases.items():
+            with self.subTest(fields=fields):
+                class X(Structure):
+                    _pack_ = 1
+                    _fields_ = list(fields)
+                self.assertEqual(sizeof(X), size)
+
     def test_anon_bitfields(self):
         # anonymous bit-fields gave a strange error message
         class X(Structure):
diff --git a/Misc/NEWS.d/next/Library/2020-05-02-01-01-30.bpo-29753.n2M-AF.rst b/Misc/NEWS.d/next/Library/2020-05-02-01-01-30.bpo-29753.n2M-AF.rst
new file mode 100644
index 0000000000000..f2a2842239b9c
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2020-05-02-01-01-30.bpo-29753.n2M-AF.rst
@@ -0,0 +1 @@
+In ctypes, now packed bitfields are calculated properly and the first item of packed bitfields is now shrank correctly.
diff --git a/Modules/_ctypes/cfield.c b/Modules/_ctypes/cfield.c
index 5bd96f1eb8c18..d96a1b0651baa 100644
--- a/Modules/_ctypes/cfield.c
+++ b/Modules/_ctypes/cfield.c
@@ -71,6 +71,18 @@ PyCField_FromDesc(PyObject *desc, Py_ssize_t index,
         Py_DECREF(self);
         return NULL;
     }
+
+#ifndef MS_WIN32
+    /* if we have a packed bitfield, calculate the minimum number of bytes we
+    need to fit it. otherwise use the specified size. */
+    if (pack && bitsize) {
+        size = (bitsize - 1) / 8 + 1;
+    } else
+#endif
+        size = dict->size;
+
+    proto = desc;
+
     if (bitsize /* this is a bitfield request */
         && *pfield_size /* we have a bitfield open */
 #ifdef MS_WIN32
@@ -87,7 +99,9 @@ PyCField_FromDesc(PyObject *desc, Py_ssize_t index,
     } else if (bitsize /* this is a bitfield request */
         && *pfield_size /* we have a bitfield open */
         && dict->size * 8 >= *pfield_size
-        && (*pbitofs + bitsize) <= dict->size * 8) {
+        /* if this is a packed bitfield, always expand it.
+           otherwise calculate if we need to expand it. */
+        && (((*pbitofs + bitsize) <= dict->size * 8) || pack)) {
         /* expand bit field */
         fieldtype = EXPAND_BITFIELD;
 #endif
@@ -95,7 +109,9 @@ PyCField_FromDesc(PyObject *desc, Py_ssize_t index,
         /* start new bitfield */
         fieldtype = NEW_BITFIELD;
         *pbitofs = 0;
-        *pfield_size = dict->size * 8;
+        /* use our calculated size (size) instead of type size (dict->size),
+           which can be different for packed bitfields */
+        *pfield_size = size * 8;
     } else {
         /* not a bit field */
         fieldtype = NO_BITFIELD;
@@ -103,9 +119,6 @@ PyCField_FromDesc(PyObject *desc, Py_ssize_t index,
         *pfield_size = 0;
     }
 
-    size = dict->size;
-    proto = desc;
-
     /*  Field descriptors for 'c_char * n' are be scpecial cased to
         return a Python string instead of an Array object instance...
     */
@@ -170,10 +183,16 @@ PyCField_FromDesc(PyObject *desc, Py_ssize_t index,
         break;
 
     case EXPAND_BITFIELD:
-        *poffset += dict->size - *pfield_size/8;
-        *psize += dict->size - *pfield_size/8;
+        /* increase the size if it is a packed bitfield.
+           EXPAND_BITFIELD should not be selected for non-packed fields if the
+           current size isn't already enough. */
+        if (pack)
+            size = (*pbitofs + bitsize - 1) / 8 + 1;
+
+        *poffset += size - *pfield_size/8;
+        *psize += size - *pfield_size/8;
 
-        *pfield_size = dict->size * 8;
+        *pfield_size = size * 8;
 
         if (big_endian)
             self->size = (bitsize << 16) + *pfield_size - *pbitofs - bitsize;