Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bpo-29843: raise AttributeError if given negative _length_ #10029

Merged
merged 4 commits into from
Oct 22, 2018
Merged

bpo-29843: raise AttributeError if given negative _length_ #10029

merged 4 commits into from
Oct 22, 2018

Conversation

taleinat
Copy link
Contributor

@taleinat taleinat commented Oct 21, 2018 •
edited

This is based on @orenmn's PR #3822.

The tests are the same, but this uses _PyLong_Sign() to check for negative values rather than PyLong_AsLongAndOverflow().

This raises AttributeError since the existing code already raises that for other invalid values (non-integers).

https://bugs.python.org/issue29843

@taleinat taleinat added the type-bug An unexpected behavior, bug, or error label Oct 21, 2018
@serhiy-storchaka
Copy link
Member

AttributeError doesn't look a correct exception. I would expect AttributeError if the attribute doesn't exist, TypeError if it has wrong type, and ValueError if it has wrong (negative) value.

Also PyObject_GetAttrString() can raise a MemoryError. Actually it can raise an arbitrary exception. They shouldn't change their type. Only AttributeError can be overridden by other AttributeError with different error message.

@serhiy-storchaka
Copy link
Member

The issue number looks wrong.

@taleinat taleinat changed the title bpo-29743: raise AttributeError if given negative _length_ bpo-29843: raise AttributeError if given negative _length_ Oct 21, 2018
@taleinat
Copy link
Contributor Author

The issue number looks wrong.

Fixed.

@taleinat taleinat force-pushed the bpo-29743/ctypes_array_improve_rejecting_negative_length branch from ba7d522 to e8d54b5 Compare October 21, 2018 15:37
@taleinat
Copy link
Contributor Author

@serhiy-storchaka, I've addressed the exception handling and raised exception types as per your comment. Please take another look!

serhiy-storchaka
serhiy-storchaka reviewed Oct 21, 2018
View reviewed changes
Lib/ctypes/test/test_arrays.py Outdated
with self.assertRaises(OverflowError):

def test_bad_length(self):
import sys
Copy link
Member

@serhiy-storchaka serhiy-storchaka Oct 21, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is imported at the top of the file.

Misc/NEWS.d/next/Core and Builtins/2018-10-21-17-43-48.bpo-29743.aeCcKR.rst Outdated
@@ -0,0 +1,4 @@
Raise `ValueError` instead of `OverflowError` in case of a negative
``_length_`` in a `ctypes.Array` subclass. Also raise `TypeError`
instead of `OverflowError` for non-integer ``_length_``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"instead of AttributeError".

Misc/NEWS.d/next/Core and Builtins/2018-10-21-17-43-48.bpo-29743.aeCcKR.rst Outdated
@@ -0,0 +1,4 @@
Raise `ValueError` instead of `OverflowError` in case of a negative
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some issues with using the default role. It is better to use :exc:`ValueError` or ``ValueError`` or just remove mark up.

Modules/_ctypes/_ctypes.c Outdated
"which must be a positive integer");
Py_XDECREF(length_attr);
if (!length_attr) {
if (!PyErr_Occurred() ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyErr_Occurred() should always return true here. Remove this redundant check.

Modules/_ctypes/_ctypes.c Outdated
Py_XDECREF(length_attr);
if (!length_attr) {
if (!PyErr_Occurred() ||
PyErr_ExceptionMatches(PyExc_AttributeError))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is worth to add similar check for _type_.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make a new PR for that. Should I also make a new issue?

@taleinat
Copy link
Contributor Author

@serhiy-storchaka, I've made the requested changes.

serhiy-storchaka
serhiy-storchaka approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM. Just a tiny nit.

Modules/_ctypes/_ctypes.c Outdated
"which must be a positive integer");
Py_XDECREF(length_attr);
if (!length_attr) {
if (PyErr_ExceptionMatches(PyExc_AttributeError))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For PEP 7 put { on the same line as if.

Modules/_ctypes/_ctypes.c Outdated
"which must be a positive integer");
Py_XDECREF(length_attr);
if (!length_attr) {
if (PyErr_ExceptionMatches(PyExc_AttributeError))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For PEP 7 put { on the same line as if. It should be on a new line only for multiline conditions.

serhiy-storchaka
serhiy-storchaka approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM. Just a tiny nit.

serhiy-storchaka
serhiy-storchaka approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM. Just a tiny nit.

serhiy-storchaka
serhiy-storchaka approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a tiny nit.

Modules/_ctypes/_ctypes.c Outdated
"which must be a positive integer");
Py_XDECREF(length_attr);
if (!length_attr) {
if (PyErr_ExceptionMatches(PyExc_AttributeError))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For PEP 7 put { on the same line as if. It should be on a new line only for multiline conditions.

Modules/_ctypes/_ctypes.c Outdated
"which must be a positive integer");
Py_XDECREF(length_attr);
if (!length_attr) {
if (PyErr_ExceptionMatches(PyExc_AttributeError))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For PEP 7 put { on the same line as if. It should be on a new line only for multiline conditions.

serhiy-storchaka
serhiy-storchaka approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a nitpick.

@taleinat
Copy link
Contributor Author

Should this be backported, and if so, how far back?

vstinner
vstinner approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@vstinner
Copy link
Member

Should this be backported, and if so, how far back?

https://bugs.python.org/issue29843#msg328216

vstinner
vstinner approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@vstinner
Copy link
Member

Hum, GitHub is sick today. I approved the PR again :-)

@serhiy-storchaka
Copy link
Member

All right until GitHub allow to merge the PR twice. 😉

@taleinat taleinat mentioned this pull request Oct 22, 2018
Closed
@taleinat taleinat merged commit 2447773 into python:master Oct 22, 2018
@taleinat taleinat deleted the bpo-29743/ctypes_array_improve_rejecting_negative_length branch October 22, 2018 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@taleinat @serhiy-storchaka @vstinner @the-knights-who-say-ni @bedevere-bot