From 1962567aac71c16604a405c060b9a11ec6a82646 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Melissa=F0=9F=A6=92?= Date: Sun, 17 Nov 2024 13:21:35 -0600 Subject: [PATCH 01/15] Fix rejection when non-bitfield fields are larger than 2^16 bytes --- Modules/_ctypes/cfield.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/Modules/_ctypes/cfield.c b/Modules/_ctypes/cfield.c index 3220852c8398e0..dfcd3783a669ff 100644 --- a/Modules/_ctypes/cfield.c +++ b/Modules/_ctypes/cfield.c @@ -110,8 +110,21 @@ PyCField_new_impl(PyTypeObject *type, PyObject *name, PyObject *proto, goto error; } - Py_ssize_t bit_size = NUM_BITS(size); - if (bit_size) { + if (bit_size_obj != Py_None) { + + Py_ssize_t bit_size; + + if (PyLong_Check(bit_size_obj)) { + bit_size = PyLong_AsSsize_t(bit_size_obj); + } else { + PyErr_Format( + PyExc_ValueError, + "bit size of field %R must be an integer size for bit fields", + self->name + ); + goto error; + } + assert(bit_size > 0); assert(bit_size <= info->size * 8); switch(info->ffi_type_pointer.type) { From 2bd972b180e5d750cb5140171b9df35677ecb8f7 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sun, 17 Nov 2024 21:35:56 +0000 Subject: [PATCH 02/15] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2024-11-17-21-35-55.gh-issue-126937.qluVM0.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-11-17-21-35-55.gh-issue-126937.qluVM0.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-11-17-21-35-55.gh-issue-126937.qluVM0.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-17-21-35-55.gh-issue-126937.qluVM0.rst new file mode 100644 index 00000000000000..f579444db58730 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-17-21-35-55.gh-issue-126937.qluVM0.rst @@ -0,0 +1 @@ +Fixed TypeError when a Structure's field's size is >65535 bytes From d2954cdefdbe3608116c425796b134ff53fd1d60 Mon Sep 17 00:00:00 2001 From: Terry Jan Reedy Date: Sun, 17 Nov 2024 18:25:18 -0500 Subject: [PATCH 03/15] Update Modules/_ctypes/cfield.c --- Modules/_ctypes/cfield.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Modules/_ctypes/cfield.c b/Modules/_ctypes/cfield.c index dfcd3783a669ff..7374df679f4834 100644 --- a/Modules/_ctypes/cfield.c +++ b/Modules/_ctypes/cfield.c @@ -124,7 +124,6 @@ PyCField_new_impl(PyTypeObject *type, PyObject *name, PyObject *proto, ); goto error; } - assert(bit_size > 0); assert(bit_size <= info->size * 8); switch(info->ffi_type_pointer.type) { From 0d2122978662f9efc676c22c2c90f0beeda95f30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Melissa=F0=9F=A6=92?= Date: Mon, 18 Nov 2024 07:43:55 -0600 Subject: [PATCH 04/15] Remove specific dependency on Python 3.12 for black --- .pre-commit-config.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index ec769d7ff70314..e1196f20ad68b9 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -29,12 +29,12 @@ repos: - id: black name: Run Black on Tools/build/check_warnings.py files: ^Tools/build/check_warnings.py - language_version: python3.12 + language_version: python3 args: [--line-length=79] - id: black name: Run Black on Tools/jit/ files: ^Tools/jit/ - language_version: python3.12 + language_version: python3 - repo: https://github.com/pre-commit/pre-commit-hooks rev: v5.0.0 From d8a6f080eaaa70a977c8700243a28f492076258d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Melissa=F0=9F=A6=92?= Date: Mon, 18 Nov 2024 07:47:54 -0600 Subject: [PATCH 05/15] Add test case for large fields --- Lib/test/test_ctypes/test_struct_fields.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Lib/test/test_ctypes/test_struct_fields.py b/Lib/test/test_ctypes/test_struct_fields.py index b5e165f3bae929..6a1011cb3f792b 100644 --- a/Lib/test/test_ctypes/test_struct_fields.py +++ b/Lib/test/test_ctypes/test_struct_fields.py @@ -75,6 +75,12 @@ def __init_subclass__(cls, **kwargs): 'ctypes state is not initialized'): class Subclass(BrokenStructure): ... + def test_gh126937(self): + class X(self.cls): + _fields_ = [('char', c_char),] + class Y(self.cls): + _fields_ = [('largeField', X * (2 ** 32))] + # __set__ and __get__ should raise a TypeError in case their self # argument is not a ctype instance. def test___set__(self): From 7402a8f60767694f2b335a8f855d2cdbdc1df7e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Melissa=F0=9F=A6=92?= Date: Mon, 18 Nov 2024 08:19:47 -0600 Subject: [PATCH 06/15] Reduce test field size to (2^31 - 1) to support 32 bit systems --- Lib/test/test_ctypes/test_struct_fields.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_ctypes/test_struct_fields.py b/Lib/test/test_ctypes/test_struct_fields.py index 6a1011cb3f792b..7f29ce0e898454 100644 --- a/Lib/test/test_ctypes/test_struct_fields.py +++ b/Lib/test/test_ctypes/test_struct_fields.py @@ -79,7 +79,7 @@ def test_gh126937(self): class X(self.cls): _fields_ = [('char', c_char),] class Y(self.cls): - _fields_ = [('largeField', X * (2 ** 32))] + _fields_ = [('largeField', X * (2 ** 32 - 1))] # __set__ and __get__ should raise a TypeError in case their self # argument is not a ctype instance. From db171e04a3a6a323e398591b4e235ffc989f15ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Melissa=F0=9F=A6=92?= Date: Mon, 18 Nov 2024 08:24:11 -0600 Subject: [PATCH 07/15] Reduce test field size to (2^31 - 1) to support 32 bit systems --- Lib/test/test_ctypes/test_struct_fields.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_ctypes/test_struct_fields.py b/Lib/test/test_ctypes/test_struct_fields.py index 7f29ce0e898454..fed5ce7727db42 100644 --- a/Lib/test/test_ctypes/test_struct_fields.py +++ b/Lib/test/test_ctypes/test_struct_fields.py @@ -79,7 +79,8 @@ def test_gh126937(self): class X(self.cls): _fields_ = [('char', c_char),] class Y(self.cls): - _fields_ = [('largeField', X * (2 ** 32 - 1))] + # (2^31 - 1) is the largest size that can fit into a signed 32 bit int + _fields_ = [('largeField', X * (2 ** 31 - 1))] # __set__ and __get__ should raise a TypeError in case their self # argument is not a ctype instance. From a3bd1714b2873a83a1433288c67da28f9adcc5e7 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 4 Dec 2024 11:08:01 +0100 Subject: [PATCH 08/15] Remove pre-commit config (moved to #126971) --- .pre-commit-config.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f0c38f94a2fc35..ccaf2390d99fae 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -29,12 +29,12 @@ repos: - id: black name: Run Black on Tools/build/check_warnings.py files: ^Tools/build/check_warnings.py - language_version: python3 + language_version: python3.12 args: [--line-length=79] - id: black name: Run Black on Tools/jit/ files: ^Tools/jit/ - language_version: python3 + language_version: python3.12 - repo: https://github.com/pre-commit/pre-commit-hooks rev: v5.0.0 From 5e8aec0419d69a02b179d20412ff4b1719c05662 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 4 Dec 2024 11:20:59 +0100 Subject: [PATCH 09/15] Use PyLong_AsSsize_t to convert to Py_ssize_t --- Modules/_ctypes/stgdict.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_ctypes/stgdict.c b/Modules/_ctypes/stgdict.c index 5dbbe0b3285d58..5ca5b62427600d 100644 --- a/Modules/_ctypes/stgdict.c +++ b/Modules/_ctypes/stgdict.c @@ -292,7 +292,7 @@ PyCStructUnionType_update_stginfo(PyObject *type, PyObject *fields, int isStruct if (!tmp) { goto error; } - Py_ssize_t total_align = PyLong_AsInt(tmp); + Py_ssize_t total_align = PyLong_AsSsize_t(tmp); Py_DECREF(tmp); if (total_align < 0) { if (!PyErr_Occurred()) { @@ -306,7 +306,7 @@ PyCStructUnionType_update_stginfo(PyObject *type, PyObject *fields, int isStruct if (!tmp) { goto error; } - Py_ssize_t total_size = PyLong_AsInt(tmp); + Py_ssize_t total_size = PyLong_AsSsize_t(tmp); Py_DECREF(tmp); if (total_size < 0) { if (!PyErr_Occurred()) { From 7d3bb107a7376aab67848c05726761bc70d2ad82 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 4 Dec 2024 11:24:20 +0100 Subject: [PATCH 10/15] Test maximum field size (sys.maxsize / 8, so we can count bits later) --- Lib/test/test_ctypes/test_struct_fields.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_ctypes/test_struct_fields.py b/Lib/test/test_ctypes/test_struct_fields.py index fed5ce7727db42..c46ce0d1eeac2a 100644 --- a/Lib/test/test_ctypes/test_struct_fields.py +++ b/Lib/test/test_ctypes/test_struct_fields.py @@ -1,4 +1,5 @@ import unittest +import sys from ctypes import Structure, Union, sizeof, c_char, c_int from ._support import (CField, Py_TPFLAGS_DISALLOW_INSTANTIATION, Py_TPFLAGS_IMMUTABLETYPE) @@ -78,9 +79,12 @@ class Subclass(BrokenStructure): ... def test_gh126937(self): class X(self.cls): _fields_ = [('char', c_char),] + max_field_size = sys.maxsize // 8 class Y(self.cls): - # (2^31 - 1) is the largest size that can fit into a signed 32 bit int - _fields_ = [('largeField', X * (2 ** 31 - 1))] + _fields_ = [('largeField', X * max_field_size)] + with self.assertRaises(ValueError): + class TooBig(self.cls): + _fields_ = [('largeField', X * (max_field_size + 1))] # __set__ and __get__ should raise a TypeError in case their self # argument is not a ctype instance. From 946f26107a913ff77bd39557962d77ca60139e70 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 4 Dec 2024 11:25:50 +0100 Subject: [PATCH 11/15] Rework the asserts. --- Modules/_ctypes/cfield.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/Modules/_ctypes/cfield.c b/Modules/_ctypes/cfield.c index 7374df679f4834..2b9e8a1a10d6f5 100644 --- a/Modules/_ctypes/cfield.c +++ b/Modules/_ctypes/cfield.c @@ -111,21 +111,15 @@ PyCField_new_impl(PyTypeObject *type, PyObject *name, PyObject *proto, } if (bit_size_obj != Py_None) { - - Py_ssize_t bit_size; - - if (PyLong_Check(bit_size_obj)) { - bit_size = PyLong_AsSsize_t(bit_size_obj); - } else { - PyErr_Format( - PyExc_ValueError, - "bit size of field %R must be an integer size for bit fields", - self->name - ); - goto error; - } +#ifdef Py_DEBUG + Py_ssize_t bit_size = NUM_BITS(size); assert(bit_size > 0); assert(bit_size <= info->size * 8); + // Currently, the bit size is specified redundantly + // in NUM_BITS(size) and bit_size_obj. + // Verify that they match. + assert(PyLong_AsSsize_t(bit_size_obj) == bit_size); +#endif switch(info->ffi_type_pointer.type) { case FFI_TYPE_UINT8: case FFI_TYPE_UINT16: From 19c1b3be326fdd2a1b508f7a82e1c45eb5052a9a Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 4 Dec 2024 11:27:37 +0100 Subject: [PATCH 12/15] Test naked c_char as well --- Lib/test/test_ctypes/test_struct_fields.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Lib/test/test_ctypes/test_struct_fields.py b/Lib/test/test_ctypes/test_struct_fields.py index c46ce0d1eeac2a..a4ad72598b6410 100644 --- a/Lib/test/test_ctypes/test_struct_fields.py +++ b/Lib/test/test_ctypes/test_struct_fields.py @@ -82,9 +82,14 @@ class X(self.cls): max_field_size = sys.maxsize // 8 class Y(self.cls): _fields_ = [('largeField', X * max_field_size)] + class Z(self.cls): + _fields_ = [('largeField', c_char * max_field_size)] with self.assertRaises(ValueError): class TooBig(self.cls): _fields_ = [('largeField', X * (max_field_size + 1))] + with self.assertRaises(ValueError): + class TooBig(self.cls): + _fields_ = [('largeField', c_char * (max_field_size + 1))] # __set__ and __get__ should raise a TypeError in case their self # argument is not a ctype instance. From dcf97f2b1c53fce8253e853300dead00eb853443 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 4 Dec 2024 11:28:18 +0100 Subject: [PATCH 13/15] Better name for the test --- Lib/test/test_ctypes/test_struct_fields.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_ctypes/test_struct_fields.py b/Lib/test/test_ctypes/test_struct_fields.py index a4ad72598b6410..55af2d93c51f29 100644 --- a/Lib/test/test_ctypes/test_struct_fields.py +++ b/Lib/test/test_ctypes/test_struct_fields.py @@ -76,7 +76,7 @@ def __init_subclass__(cls, **kwargs): 'ctypes state is not initialized'): class Subclass(BrokenStructure): ... - def test_gh126937(self): + def test_max_field_size_gh126937(self): class X(self.cls): _fields_ = [('char', c_char),] max_field_size = sys.maxsize // 8 From 03bc32b33b3d9da23a4cf16a7ffb648b67d277b3 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 4 Dec 2024 11:32:27 +0100 Subject: [PATCH 14/15] Add comment --- Lib/test/test_ctypes/test_struct_fields.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Lib/test/test_ctypes/test_struct_fields.py b/Lib/test/test_ctypes/test_struct_fields.py index 55af2d93c51f29..1b3e64efd410b8 100644 --- a/Lib/test/test_ctypes/test_struct_fields.py +++ b/Lib/test/test_ctypes/test_struct_fields.py @@ -77,13 +77,20 @@ def __init_subclass__(cls, **kwargs): class Subclass(BrokenStructure): ... def test_max_field_size_gh126937(self): + # Classes for big structs should be created successfully. + # (But they most likely can't be instantiated.) + # Here we test the exact limit: the number of *bits* must fit + # in Py_ssize_t. + class X(self.cls): _fields_ = [('char', c_char),] max_field_size = sys.maxsize // 8 + class Y(self.cls): _fields_ = [('largeField', X * max_field_size)] class Z(self.cls): _fields_ = [('largeField', c_char * max_field_size)] + with self.assertRaises(ValueError): class TooBig(self.cls): _fields_ = [('largeField', X * (max_field_size + 1))] From f61faa680c8f41f4de63408f5880e6493eee0912 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Wed, 4 Dec 2024 11:32:59 +0100 Subject: [PATCH 15/15] Reword changelog Co-Authorsd-By: Peter Bierma --- .../2024-11-17-21-35-55.gh-issue-126937.qluVM0.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-11-17-21-35-55.gh-issue-126937.qluVM0.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-17-21-35-55.gh-issue-126937.qluVM0.rst index f579444db58730..8d7da0d4107021 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2024-11-17-21-35-55.gh-issue-126937.qluVM0.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-17-21-35-55.gh-issue-126937.qluVM0.rst @@ -1 +1,3 @@ -Fixed TypeError when a Structure's field's size is >65535 bytes +Fix :exc:`TypeError` when a :class:`ctypes.Structure` has a field size +that doesn't fit into an unsigned 16-bit integer. +Instead, the maximum number of *bits* is :data:`sys.maxsize`.