-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-126937: ctypes: fix TypeError when a field's size is >65535 bytes #126938
base: main
Are you sure you want to change the base?
Conversation
Fix rejection when non-bitfield fields are larger than 2^16 bytes
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 |
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 |
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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments :)
@@ -0,0 +1 @@ | |||
Fixed TypeError when a Structure's field's size is >65535 bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Modules/_ctypes/cfield.c
Outdated
Py_ssize_t bit_size = NUM_BITS(size); | ||
if (bit_size) { | ||
if (bit_size_obj != Py_None) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a newline
Modules/_ctypes/cfield.c
Outdated
Py_ssize_t bit_size; | ||
|
||
if (PyLong_Check(bit_size_obj)) { | ||
bit_size = PyLong_AsSsize_t(bit_size_obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyLong_AsSsize_t
can fail, you need to check for < 0 and then goto error
.
Modules/_ctypes/cfield.c
Outdated
if (bit_size) { | ||
if (bit_size_obj != Py_None) { | ||
|
||
Py_ssize_t bit_size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint is complaining about this. I'm assuming this just needs to get moved to before the if
Thank you! That's the correct fix; there's a few details to iron out. if (bit_size_obj != Py_None) {
#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) {
... (In
Yes.
It is; ideally with a comment like @Melissa0x1f992, if at any time working on this PR stops being fun for you, let me know and I can finish it. I don't know if you're driven by curiosity or just want the bug fixed :) |
Do you still want to work on this PR? |
encukou, thank you! Yeah, life's gotten busy, so I won't be around to see this through. It was a fun experience, and I hope to contribute on other things in the future 🙂 |
Co-Authorsd-By: Peter Bierma <zintensitydev@gmail.com>
OK. Thank you for the fix! |
@serhiy-storchaka, should I wait for your review? |
I am looking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is the same PR. As I said in the issue, it fixes some symptoms, but there is a deeper issue.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was an accident -- my mistake.
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be guaranteed? What if bit_size_obj
is not None and is not an int?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Yes. The important thing in this PR is the regression test (and fixing the test for now). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Used the
bit_size_obj
as seems to be intended, based on its usage in Lib/ctypes/_layout.pyTested the change locally informally. Didn't write any tests.