gh-126937: ctypes: fix TypeError when a field's size is >65535 bytes by Melissa0x1f992 · Pull Request #126938 · python/cpython · GitHub

Used the bit_size_obj as seems to be intended, based on its usage in Lib/ctypes/_layout.py

Tested the change locally informally. Didn't write any tests.

Issue: Struct fields with size > 65535 bytes throw TypeError #126937

Fix rejection when non-bitfield fields are larger than 2^16 bytes

All commit authors signed the Contributor License Agreement.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Need some help understanding the lint error on this

By somewhat random clicking of the search box and the test stage line, I got the full log. There were 6 spaces after the final \n, thus trailing spaces failed. (Note that running patchcheck as suggested somewhere would have fixed this.)

I asked to rerun just the one test, but the edit triggered all.

I can't review the actual change. It might help someone else if you could connect the change to the change in 3.14 that caused the regression. To find the latter, I would first run git blame on cfield.c

cc @encukou

Use bitsize param

Added a test. It doesn't have any assertions bc the failure case is an error getting raised. Not sure if this is the appropriate way to add such a test.

Reduce test field size to (2^31 - 1) to support 32 bit systems

For the Win32, I don't have that setup. Could I get some help determining what field size should pass the < (1ULL << (8*sizeof(Py_ssize_t)-1)) / 8) test on Win32? Am I off by one? I don't want to commit (2^31 - 1 - 1) unless there's a good reason to consider that one byte smaller the limit for Win32.

Is there a way for the test to set up different field sizes based on the current platform, and is that desirable? I'd like to test the actual max size, regardless of platform, rather than the smallest of the different platform maxes.

Some initial comments :)

Suggested change

      
            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.

No need for a newline

Suggested change

PyLong_AsSsize_t can fail, you need to check for < 0 and then goto error.

Lint is complaining about this. I'm assuming this just needs to get moved to before the if

Co-Authorsd-By: Peter Bierma <zintensitydev@gmail.com>

OK. Thank you for the fix!
I hope you don't mind me pushing to your branch -- this way I can keep changes in this PR, with the discussion.

@serhiy-storchaka, should I wait for your review?

I am looking.

Ah, this is the same PR. As I said in the issue, it fixes some symptoms, but there is a deeper issue.

What was a reason for using PyLong_AsInt(). This is a new API, so it unlikely was accident. Is there a limitation on total_size to be in the range of int?

It was an accident -- my mistake.

Can this be guaranteed? What if bit_size_obj is not None and is not an int?

It can.
The implementation is split between internal Python code in ctypes._layout, and C code here. They are currently tightly coupled. The assert also verifies the Python part.

As I said in the issue, it fixes some symptoms, but there is a deeper issue.

Yes. The important thing in this PR is the regression test (and fixing the test for now).
I plan to improve the code further: see #127297 for the next step, a refactoring to make future work easier. After that's in I plan to split up the size and bitfield info.

LGTM

Thanks @Melissa0x1f992 for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

Thanks @Melissa0x1f992 for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

Sorry, @Melissa0x1f992 and @encukou, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker cef0a90d8f3a94aa534593f39b4abf98165675b9 3.13

Sorry, @Melissa0x1f992 and @encukou, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker cef0a90d8f3a94aa534593f39b4abf98165675b9 3.12

…ythonGH-126938) This backports the test from pythonGH-126938, with changed limit and exception class. Co-authored-by: Melissa0x1f992 <70096546+Melissa0x1f992@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu> Co-authored-by: Petr Viktorin <encukou@gmail.com>

GH-127825 is a backport of this pull request to the 3.13 branch.

…GH-126938) (GH-127825) This backports the *test* from GH-126938, with changed limit and exception class. Co-authored-by: Melissa0x1f992 <70096546+Melissa0x1f992@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>

… field (pythonGH-126938) (pythonGH-127825) This backports the *test* from pythonGH-126938, with changed limit and exception class. Co-authored-by: Melissa0x1f992 <70096546+Melissa0x1f992@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu> (cherry-picked from d51c144)

…GH-126938) (GH-127825) (GH-127909) This backports the *test* from GH-126938, with changed limit and exception class. Co-authored-by: Melissa0x1f992 <70096546+Melissa0x1f992@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu> (cherry-picked from d51c144)

…bytes (pythonGH-126938) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu> Co-authored-by: Petr Viktorin <encukou@gmail.com>

Melissa0x1f992 and others added 2 commits November 17, 2024 13:21

Fix rejection when non-bitfield fields are larger than 2^16 bytes

1962567

Merge pull request #1 from Melissa0x1f992/use-bitsize-param

1900cf5

Fix rejection when non-bitfield fields are larger than 2^16 bytes

bedevere-app Bot added the awaiting review label Nov 17, 2024

bedevere-app Bot mentioned this pull request Nov 17, 2024

Struct fields with size > 65535 bytes throw TypeError #126937

Closed

📜🤖 Added by blurb_it.

2bd972b

terryjreedy reviewed Nov 17, 2024

View reviewed changes

Comment thread Modules/_ctypes/cfield.c Outdated

Update Modules/_ctypes/cfield.c

d2954cd

Melissa0x1f992 and others added 3 commits November 18, 2024 07:43

Remove specific dependency on Python 3.12 for black

0d21229

Add test case for large fields

d8a6f08

Merge pull request #2 from Melissa0x1f992/use-bitsize-param

16bfb44

Use bitsize param

Melissa0x1f992 requested review from AlexWaygood and hugovk as code owners November 18, 2024 13:51

Melissa0x1f992 and others added 4 commits November 18, 2024 08:19

Reduce test field size to (2^31 - 1) to support 32 bit systems

7402a8f

Merge branch 'python:main' into main

8994ad0

Reduce test field size to (2^31 - 1) to support 32 bit systems

db171e0

Merge pull request #3 from Melissa0x1f992/use-bitsize-param

7bc0ceb

Reduce test field size to (2^31 - 1) to support 32 bit systems

AlexWaygood reviewed Nov 18, 2024

View reviewed changes

Comment thread .pre-commit-config.yaml

ZeroIntensity reviewed Nov 18, 2024

View reviewed changes

ZeroIntensity added the topic-ctypes label Nov 18, 2024

hugovk reviewed Nov 18, 2024

View reviewed changes

Comment thread .pre-commit-config.yaml Outdated

Comment thread .pre-commit-config.yaml Outdated

vstinner changed the title gh-126937: Fix TypeError when a field's size is >65535 bytes gh-126937, ctypes: Fix TypeError when a field's size is >65535 bytes Nov 19, 2024

Reword changelog

f61faa6

Co-Authorsd-By: Peter Bierma <zintensitydev@gmail.com>

serhiy-storchaka reviewed Dec 9, 2024

View reviewed changes

vstinner approved these changes Dec 9, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels Dec 9, 2024

serhiy-storchaka approved these changes Dec 9, 2024

View reviewed changes

encukou merged commit cef0a90 into python:main Dec 10, 2024

bedevere-app Bot removed the awaiting merge label Dec 10, 2024

serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Dec 10, 2024

miss-islington-app Bot assigned encukou Dec 10, 2024

serhiy-storchaka removed needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Dec 10, 2024

encukou mentioned this pull request Dec 11, 2024

[3.13] gh-126937: ctypes: add test for maximum size of a struct field (GH-126938) #127825

Merged

encukou mentioned this pull request Dec 13, 2024

[3.12] gh-126937: ctypes: add test for maximum size of a struct field (GH-126938) (GH-127825) #127909

Merged

encukou mentioned this pull request Jan 17, 2025

gh-128715: Expose ctypes.CField, with info attributes #128950

Merged

+                      // 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);

		@@ -0,0 +1 @@
		Fixed TypeError when a Structure's field's size is >65535 bytes

	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.

-                  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);

                       goto error;
                   }
-                  Py_ssize_t total_size = PyLong_AsInt(tmp);
+                  Py_ssize_t total_size = PyLong_AsSsize_t(tmp);

gh-126937: ctypes: fix TypeError when a field's size is >65535 bytes by Melissa0x1f992 · Pull Request #126938 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-126937: ctypes: fix TypeError when a field's size is >65535 bytes - #126938

gh-126937: ctypes: fix TypeError when a field's size is >65535 bytes#126938encukou merged 20 commits intopython:mainpython/cpython:mainfrom Melissa0x1f992:mainMelissa0x1f992/cpython:mainCopy head branch name to clipboard

Conversation

Melissa0x1f992 commented Nov 17, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 17, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app Bot commented Nov 17, 2024

Uh oh!

Melissa0x1f992 commented Nov 17, 2024

Uh oh!

Uh oh!

terryjreedy commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryjreedy commented Nov 17, 2024

Uh oh!

picnixz commented Nov 18, 2024

Uh oh!

Melissa0x1f992 commented Nov 18, 2024

Uh oh!

Uh oh!

Melissa0x1f992 commented Nov 18, 2024

Uh oh!

Melissa0x1f992 commented Nov 18, 2024

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

encukou commented Dec 4, 2024

Uh oh!

encukou commented Dec 9, 2024

Uh oh!

serhiy-storchaka commented Dec 9, 2024

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

encukou Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

encukou Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

encukou commented Dec 9, 2024

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app Bot commented Dec 10, 2024

Uh oh!

miss-islington-app Bot commented Dec 10, 2024

Uh oh!

miss-islington-app Bot commented Dec 10, 2024

gh-126937: ctypes: fix TypeError when a field's size is >65535 bytes#126938
encukou merged 20 commits into
python:mainfrom
Melissa0x1f992:main

Melissa0x1f992 commented Nov 17, 2024 •
edited by bedevere-app Bot
Loading

ghost commented Nov 17, 2024 •
edited by ghost
Loading

terryjreedy commented Nov 17, 2024 •
edited
Loading

ZeroIntensity Nov 18, 2024 •
edited
Loading