bpo-29843: raise AttributeError if given negative _length_ by taleinat · Pull Request #10029 · python/cpython

taleinat · 2018-10-21T14:51:41Z

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

serhiy-storchaka · 2018-10-21T15:09:23Z

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 · 2018-10-21T15:10:10Z

The issue number looks wrong.

taleinat · 2018-10-21T15:21:00Z

The issue number looks wrong.

Fixed.

Co-authored-by: Oren Milman <[email protected]>

taleinat · 2018-10-21T15:37:56Z

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

serhiy-storchaka · 2018-10-21T15:53:55Z

-        with self.assertRaises(OverflowError):
+
+    def test_bad_length(self):
+        import sys


It is imported at the top of the file.

serhiy-storchaka · 2018-10-21T15:59:53Z

@@ -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_``.


"instead of AttributeError".

serhiy-storchaka · 2018-10-21T16:00:17Z

@@ -0,0 +1,4 @@
+Raise `ValueError` instead of `OverflowError` in case of a negative


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

serhiy-storchaka · 2018-10-21T16:01:16Z

-                        "which must be a positive integer");
-        Py_XDECREF(length_attr);
+    if (!length_attr) {
+        if (!PyErr_Occurred() ||


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

serhiy-storchaka · 2018-10-21T16:03:47Z

-        Py_XDECREF(length_attr);
+    if (!length_attr) {
+        if (!PyErr_Occurred() ||
+            PyErr_ExceptionMatches(PyExc_AttributeError))


It is worth to add similar check for _type_.

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

taleinat · 2018-10-21T17:54:31Z

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

serhiy-storchaka

Thank you. LGTM. Just a tiny nit.

serhiy-storchaka · 2018-10-22T04:42:50Z

-                        "which must be a positive integer");
-        Py_XDECREF(length_attr);
+    if (!length_attr) {
+        if (PyErr_ExceptionMatches(PyExc_AttributeError))


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

serhiy-storchaka · 2018-10-22T04:44:04Z

-                        "which must be a positive integer");
-        Py_XDECREF(length_attr);
+    if (!length_attr) {
+        if (PyErr_ExceptionMatches(PyExc_AttributeError))


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

serhiy-storchaka

Thank you. LGTM. Just a tiny nit.

serhiy-storchaka

Thank you. LGTM. Just a tiny nit.

serhiy-storchaka

LGTM. Just a tiny nit.

serhiy-storchaka · 2018-10-22T05:09:24Z

-                        "which must be a positive integer");
-        Py_XDECREF(length_attr);
+    if (!length_attr) {
+        if (PyErr_ExceptionMatches(PyExc_AttributeError))


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

serhiy-storchaka · 2018-10-22T05:09:39Z

-                        "which must be a positive integer");
-        Py_XDECREF(length_attr);
+    if (!length_attr) {
+        if (PyErr_ExceptionMatches(PyExc_AttributeError))


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

serhiy-storchaka

LGTM. Just a nitpick.

taleinat · 2018-10-22T13:49:24Z

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

vstinner

LGTM.

vstinner · 2018-10-22T14:07:04Z

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

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

vstinner

LGTM.

vstinner · 2018-10-22T14:13:32Z

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

serhiy-storchaka · 2018-10-22T14:18:08Z

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

taleinat added the type-bug An unexpected behavior, bug, or error label Oct 21, 2018

taleinat requested review from serhiy-storchaka and vstinner October 21, 2018 14:51

the-knights-who-say-ni added the CLA signed label Oct 21, 2018

bedevere-bot added the awaiting merge label Oct 21, 2018

taleinat changed the title ~~bpo-29743: raise AttributeError if given negative _length_~~ bpo-29843: raise AttributeError if given negative _length_ Oct 21, 2018

taleinat and others added 2 commits October 21, 2018 18:36

bpo-29843: raise AttributeError if given negative _length_

bdf4679

Co-authored-by: Oren Milman <[email protected]>

bpo-29843: raise TypeError or ValueError when appropriate

e8d54b5

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

serhiy-storchaka reviewed Oct 21, 2018

View reviewed changes

bpo-29843: clean up follow code review

bc75e8f

serhiy-storchaka approved these changes Oct 22, 2018

View reviewed changes

bpo-29843: minor code style fix

afe039e

serhiy-storchaka approved these changes Oct 22, 2018

View reviewed changes

vstinner approved these changes Oct 22, 2018

View reviewed changes

taleinat mentioned this pull request Oct 22, 2018

bpo-29843: Raise ValueError instead of OverflowError in case of a negative _length_ in a ctypes.Array subclass #3822

Closed

taleinat merged commit 2447773 into python:master Oct 22, 2018

taleinat deleted the bpo-29743/ctypes_array_improve_rejecting_negative_length branch October 22, 2018 15:33

bedevere-bot removed the awaiting merge label Oct 22, 2018

		@@ -0,0 +1,4 @@
		Raise `ValueError` instead of `OverflowError` in case of a negative

Uh oh!

Uh oh!

Conversation

taleinat commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Oct 21, 2018

Uh oh!

serhiy-storchaka commented Oct 21, 2018

Uh oh!

taleinat commented Oct 21, 2018

Uh oh!

taleinat commented Oct 21, 2018

Uh oh!

serhiy-storchaka Oct 21, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taleinat commented Oct 21, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

taleinat commented Oct 22, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 22, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 22, 2018

Uh oh!

serhiy-storchaka commented Oct 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

taleinat commented Oct 21, 2018 •

edited

Loading

serhiy-storchaka Oct 21, 2018 •

edited

Loading