bpo-15988: make various overflow messages more consistent and helpful #668

orenmn · 2017-03-14T23:15:33Z

according to http://bugs.python.org/issue15988:

in Python/getargs.c, patch seterror so that it would be easier to do 2 in various places that use PyArg_* functions.
make various OverflowError and ValueError messages more consistent and helpful
add various overflow tests

(I ran the test module, and on my Windows 10, the same tests failed with
and without my patches. However, on my Ubuntu 16.04 VM, none of the tests
failed.)

…i added to csv, as the inconsistent message in csv was already fixed, and so my patch isn't related to it anymore

…on of the file

…oryio, and optimized an if in seterror

…riginal test

…fix my socket tests

…Arg_* funcs in mmapmodule.c

… remove redundant code in _PyLong_AsInt

…at_long_internal

serhiy-storchaka

This is a great patch! Thanks Oren.

I made the first turn of reviewing. In general LGTM, but I left a number of comments. I think some changes could be extracted to separate issues. Some OverflowErrors could be avoided at all. I didn't tested all changes. I am not sure that reraising OverflowError with new error messages is appropriate in all cases. In some cases current error message looks not obviously worse.

serhiy-storchaka · 2017-03-15T09:00:39Z

Objects/longobject.c

+    }
    if (shiftby < 0) {
-        PyErr_SetString(PyExc_ValueError, "negative shift count");
+        PyErr_SetString(PyExc_ValueError, "shift count can't be negative");


I think it would be better to raise the same ValueError for all negative values, even if they don't fit in Py_ssize_t.

This changes the behavior so maybe it worths the separate issue (or be resolved as a part of bpo-29816).

serhiy-storchaka · 2017-03-15T09:02:17Z

Objects/longobject.c

+            /* PyLong_Check(b) is true, so it must be that
+               PyLong_AsSsize_t raised an OverflowError. */
+            assert(PyErr_ExceptionMatches(PyExc_OverflowError));
+            PyErr_SetString(PyExc_OverflowError,


See bpo-29816. Right shift can be changed to never raise an OverflowError.

serhiy-storchaka · 2017-03-15T09:22:05Z

Objects/longobject.c

    if (i < 0) {
        PyErr_SetString(PyExc_OverflowError,
-                        "can't convert negative value to unsigned int");
+                        "can't convert negative Python int to C "


I think ValueError is more appropriate here.

serhiy-storchaka · 2017-03-15T09:23:33Z

Objects/longobject.c

    if (i < 0) {
        PyErr_SetString(PyExc_OverflowError,
-                   "can't convert negative value to size_t");
+                        "can't convert negative Python int to C size_t");


I think ValueError is more appropriate here.

Should we open a sub-issue of bpo-29833 for this?
IMHO, as this is very similar to bpo-29834, maybe we should just mention in bpo-29834 that in case bpo-29834 is accepted and fixed, then also PyLong_AsSize_t should also be changed in a similar way.

serhiy-storchaka · 2017-03-15T09:30:03Z

Objects/longobject.c

-    if (overflow || result > INT_MAX || result < INT_MIN) {
-        /* XXX: could be cute and give a different
-           message for overflow == -1 */
+    if (overflow == 1 || result > INT_MAX) {


In common case overflow == 0 and INT_MIN <= result <= INT_MAX. Checking three false conditions in overflow || result > INT_MAX || result < INT_MIN is faster than checking four false conditions in overflow == 1 || result > INT_MAX and overflow == -1 || result < INT_MIN (and comparing with zero can be faster than comparing with non-zero). It is better to keep the current check and adds more checks only for qualifying exception message.

serhiy-storchaka · 2017-03-15T15:16:28Z

Modules/_datetimemodule.c

    };

-    if (PyArg_ParseTupleAndKeywords(args, kw, "|OOOOOOO:__new__",
+    if (PyArg_ParseTupleAndKeywords(args, kw, "|OOOOOOO:timedelta.__new__",


This change looks unrelated to this issue. After converting this module to Argument Clinic it will be outdated.

I agree. I changed it only because I changed other similar *_new functions in Modules/_datetimemodule.c, and this :__new__ seemed like a mistake that I should correct while I am there.
(anyway, I will remove this change.)

serhiy-storchaka · 2017-03-15T15:20:13Z

Modules/_cursesmodule.c

        break;
    case 1:
-        if (!PyArg_ParseTuple(args,"i;n", &n))
+        if (!PyArg_ParseTuple(args, "i:getstr", &n)) {


This change looks unrelated to this issue. It will be outdated after converting the module to Argument Clinic.

why is it not related to this issue?
do you mean it is more related to bpo-29833?

Anyway, should I remove this change and all other similar changes I made to format arguments of PyArg_ParseTuple in _cursesmodule.c?

It doesn't directly related to error messages in case of integer overflow. Remove all similar changes. They will be made by bpo-20171.

serhiy-storchaka · 2017-03-15T15:22:18Z

Modules/_ctypes/_ctypes.c

    if (overflow) {
        PyErr_SetString(PyExc_OverflowError,
-                        "The '_length_' attribute is too large");
+                        "The '_length_' attribute does not fit in C long");


Negative length is not valid. Raise ValueError in that case.

all right. I would also add a test_bad_length test to Lib/ctypes/test/test_arrays.py.

by the way, according to https://docs.python.org/3.7/library/ctypes.html#ctypes.Array._length_,
_length_ must be positive (ISTM that it makes sense, as an array of length 0 is useless), but currently no error is raised in case _length_ == 0.
do you think this is OK?

on second thought, this changes the behavior from raising OverflowError for small negative _length_ to raising ValueError.
should I open another sub-issue of bpo-29833 for that?
or maybe should I just raise OverflowError with a better error message?

I don't know whether the length 0 is acceptable. Yes, since changing OverflowError to ValueError changes the behavior, this worths a sub-issue.

opened http://bugs.python.org/issue29843

serhiy-storchaka · 2017-03-15T15:30:39Z

Modules/_blake2/blake2s_impl.c

        if (leaf_size == (unsigned long) -1 && PyErr_Occurred()) {
+            if (PyErr_ExceptionMatches(PyExc_OverflowError)) {
+                PyErr_SetString(PyExc_OverflowError,
+                                "leaf_size must be between 0 and 2**32-1");


This error message and error message at line 169 should be same.

It may be worth to raise ValueError for negative leaf_size. Are there other restrictions for upper limit? Since this is specific to BLAKE2b it is worth to open a separate issue about this.

so do you suggest to remove the changes in py_blake2b_new_impl and py_blake2s_new_impl, and patch these functions as part of the issue of replacing OverflowError with ValueError for negative values?

I don't well known with this part of code and suggest to open a separate issue for changes in this file.

opened https://bugs.python.org/issue29835

serhiy-storchaka · 2017-03-15T15:37:38Z

Lib/test/test_stat.py


+    @cpython_only
+    def test_mode_t_converter(self):
+        from _testcapi import USHRT_MAX


This is needed only on Windows. It is worth to split the test on two parts: the general one and the Windows-specific one.

serhiy-storchaka · 2017-03-16T18:34:03Z

Modules/arraymodule.c

       must use the next size up that is signed ('h') and manually do
       the overflow checking */
-    if (!PyArg_Parse(v, "h;array item must be integer", &x))
+    if (!PyArg_Parse(v, "h:array('b').__setitem__", &x)) {


I think it is enough to keep OverflowError raised in PyArg_Parse(). It should be good enough. No change is needed.

serhiy-storchaka · 2017-03-16T18:42:48Z

Modules/_sre.c

        if ((unsigned long) self->code[i] != value) {
            PyErr_SetString(PyExc_OverflowError,
-                            "regular expression code size limit exceeded");
+                            "regular expression code out of range");


One option is to add Py_DECREF(self); and return NULL; here.

Other option is to left just break and rewrite the condition after the loop:

if (i < n || PyErr_Occurred() && PyErr_ExceptionMatches(PyExc_OverflowError)) { PyErr_SetString(PyExc_OverflowError, ...); } if (PyErr_Occurred()) { Py_DECREF(self); return NULL; }

Third option is to add a label before PyErr_SetString(PyExc_OverflowError, ...); and jump to it from here. This looks the simplest option.

serhiy-storchaka · 2017-03-16T18:47:12Z

Modules/arraymodule.c

    else if (x < -128) {
        PyErr_SetString(PyExc_OverflowError,
-            "signed char is less than minimum");
+                        "array item too small to convert to C char");


Make it consistent with error messages raised by PyArg_Parse().

serhiy-storchaka · 2017-03-16T18:48:01Z

Modules/mmapmodule.c

    if (map_size < 0) {
        PyErr_SetString(PyExc_OverflowError,
-                        "memory mapped length must be postiive");
+                        "memory mapped length must be positive");


But check that 0 is actually accepted as the length. That there is not a bug in the condition.

serhiy-storchaka · 2017-03-16T18:50:49Z

Modules/mmapmodule.c


-    if (!PyArg_ParseTupleAndKeywords(args, kwdict, "in|ziL", keywords,
-                                     &fileno, &map_size,
+    if (!PyArg_ParseTupleAndKeywords(args, kwdict, "in|ziL:mmap.__new__",


At best, it would be the same message.

serhiy-storchaka · 2017-09-24T11:14:05Z

@orenmn, could you please merge with master and resolve conflicts? This will help further reviewing this giant PR.

Some issues may be already solved, perhaps in slightly different way.

orenmn · 2017-09-25T07:24:28Z

I still have some small issues that i wish to take care of before diving into this one, but i hope to get to it sometime soon.

orenmn · 2017-09-28T06:47:27Z

This PR is currently irrelevant, as explained in https://bugs.python.org/issue15988#msg303192

brettcannon · 2018-02-21T01:51:41Z

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

csabella · 2019-05-29T01:27:28Z

Based on @orenmn's last message and the merge conflicts, I'm going to close this pull request. I believe @serhiy-storchaka was suggesting for this to be split into smaller pull requests that would be easier to review. If that's not the case, feel free to re-open this PR.

orenmn added 21 commits March 10, 2017 23:32

init commit

f9a126f

remove a redundant test of timedelta's constructor, and remove tests …

37fc195

…i added to csv, as the inconsistent message in csv was already fixed, and so my patch isn't related to it anymore

added a comment and removed a redundant test

c35fc35

solve conflict in Modules/io/bytesio.c while merging master

b6cc2ea

merged master

bba2087

remove functions names from error messages, according to the conventi…

fbb56ab

…on of the file

fixed an lzma test, removed my captain obvious comments from test_mem…

8091b62

…oryio, and optimized an if in seterror

in test_mmap: removed redundant test, and improved comments

f36f579

Unix, not unix

343b679

remove some more of my captain obvious comments

bb97755

in test_posix: move test inside @cpython_only, and undo removing an o…

34e34ab

…riginal test

remove from socketmodule.c a port boundaries check I have added, and …

a88ecc5

…fix my socket tests

add comment to test_stat

07387ec

fixed error message in blake

458dbc7

fixed array error messages

dca2943

fixed some error messages in selectmodule.c, and fixed argument to Py…

f752b12

…Arg_* funcs in mmapmodule.c

fixed some error messages in bytearrayobject.c and bytesobject.c, and…

043ea7e

… remove redundant code in _PyLong_AsInt

fix _PyLong_AsInt and remove a not-relevant-anymore comment from form…

499ffc3

…at_long_internal

remove redundant new line

82f3125

fixed a test in test_long

f0b59b7

added a skipUnless in test_posix

abb2977

the-knights-who-say-ni added the CLA signed label Mar 14, 2017

serhiy-storchaka self-requested a review March 15, 2017 05:27

serhiy-storchaka added the type-feature A feature request or enhancement label Mar 15, 2017

serhiy-storchaka requested changes Mar 15, 2017

View reviewed changes

serhiy-storchaka requested changes Mar 16, 2017

View reviewed changes

Mariatta added the needs rebase label Oct 9, 2017

Mariatta removed the needs rebase label Oct 9, 2017

brettcannon added the awaiting changes label Feb 21, 2018

csabella closed this May 29, 2019

serhiy-storchaka mentioned this pull request Dec 26, 2024

Inconsistency in overflow error messages of integer argument #60192

Open

Uh oh!

bpo-15988: make various overflow messages more consistent and helpful #668

bpo-15988: make various overflow messages more consistent and helpful #668

Uh oh!

Conversation

orenmn commented Mar 14, 2017

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!

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!

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!

orenmn Mar 17, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

orenmn Mar 17, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Sep 24, 2017

Uh oh!

orenmn commented Sep 25, 2017

Uh oh!

orenmn commented Sep 28, 2017

Uh oh!

brettcannon commented Feb 21, 2018

Uh oh!

csabella commented May 29, 2019

Uh oh!

Uh oh!

orenmn Mar 17, 2017 •

edited

Loading

orenmn Mar 17, 2017 •

edited

Loading