Skip to content

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

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

Closed
orenmn wants to merge 21 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f9a126f
init commit
orenmn Mar 10, 2017
37fc195
remove a redundant test of timedelta's constructor, and remove tests …
orenmn Mar 11, 2017
c35fc35
added a comment and removed a redundant test
orenmn Mar 11, 2017
b6cc2ea
solve conflict in Modules/io/bytesio.c while merging master
orenmn Mar 11, 2017
bba2087
merged master
orenmn Mar 13, 2017
fbb56ab
remove functions names from error messages, according to the conventi…
orenmn Mar 13, 2017
8091b62
fixed an lzma test, removed my captain obvious comments from test_mem…
orenmn Mar 13, 2017
f36f579
in test_mmap: removed redundant test, and improved comments
orenmn Mar 13, 2017
343b679
Unix, not unix
orenmn Mar 13, 2017
bb97755
remove some more of my captain obvious comments
orenmn Mar 13, 2017
34e34ab
in test_posix: move test inside @cpython_only, and undo removing an o…
orenmn Mar 13, 2017
a88ecc5
remove from socketmodule.c a port boundaries check I have added, and …
orenmn Mar 14, 2017
07387ec
add comment to test_stat
orenmn Mar 14, 2017
458dbc7
fixed error message in blake
orenmn Mar 14, 2017
dca2943
fixed array error messages
orenmn Mar 14, 2017
f752b12
fixed some error messages in selectmodule.c, and fixed argument to Py…
orenmn Mar 14, 2017
043ea7e
fixed some error messages in bytearrayobject.c and bytesobject.c, and…
orenmn Mar 14, 2017
499ffc3
fix _PyLong_AsInt and remove a not-relevant-anymore comment from form…
orenmn Mar 14, 2017
82f3125
remove redundant new line
orenmn Mar 14, 2017
f0b59b7
fixed a test in test_long
orenmn Mar 14, 2017
abb2977
added a skipUnless in test_posix
orenmn Mar 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixed some error messages in selectmodule.c, and fixed argument to Py… 
…Arg_* funcs in mmapmodule.c
  • Loading branch information
@orenmn
orenmn committed Mar 14, 2017
commit f752b12334d8694b30391aeb44b777d5b8ff200d
6 changes: 3 additions & 3 deletions Modules/mmapmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1092,7 +1092,7 @@ new_mmap_object(PyTypeObject *type, PyObject *args, PyObject *kwdict)
"access", "offset", NULL};

if (!PyArg_ParseTupleAndKeywords(args, kwdict,
"in|iii" _Py_PARSE_OFF_T ":mmap",
"in|iii" _Py_PARSE_OFF_T ":mmap.__new__",
keywords, &fd, &map_size, &flags, &prot,
&access, &offset)) {
return NULL;
Expand Down Expand Up @@ -1253,8 +1253,8 @@ new_mmap_object(PyTypeObject *type, PyObject *args, PyObject *kwdict)
"tagname",
"access", "offset", NULL };

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

@serhiy-storchaka serhiy-storchaka Mar 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change looks unrelated to this issue. It is rather related to bpo-28261.

And avoid using "dunder" names. The user directly calls mmap(), not mmap.__new__().

@orenmn orenmn Mar 16, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

why is it related to bpo-28261?

ISTM that currently the error messages are inconsistent:

>>> mmap.mmap(-1, -1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: memory mapped length must be postiive
>>> mmap.mmap(-1, -1 << 1000)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: Python int too large to convert to C ssize_t

the patches change it to:

>>> mmap.mmap(-1, -1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: memory mapped length must be non-negative
>>> mmap.mmap(-1, -1 << 1000)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: mmap() argument out of range

@serhiy-storchaka serhiy-storchaka Mar 16, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At best, it would be the same message.

@orenmn orenmn Mar 16, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can't think of a simple way to achieve that, as the overflow happens inside PyArg_ParseTupleAndKeywords, but it could happen in 4 different arguments, so new_mmap_object can't know in which of the arguments the overflow happened.

that's why I made the patch in seterror(). that was the best simple solution I could come up with.

keywords, &fileno, &map_size,
&tagname, &access, &offset)) {
return NULL;
}
Expand Down
8 changes: 4 additions & 4 deletions Modules/selectmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ poll_register(pollObject *self, PyObject *args)
assert(PyErr_Occurred());
if (PyErr_ExceptionMatches(PyExc_OverflowError)) {
PyErr_SetString(PyExc_OverflowError,
"register: eventmask value does not fit in C "
"eventmask value does not fit in C "

@serhiy-storchaka serhiy-storchaka Mar 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be easier to rename ushort_converter to eventmask_converter and change its error message. But I think that at this stage it would be better to withdraw these changes. Current error message looks enough good to me.

@orenmn orenmn Mar 16, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

all right, I will remove this change, and other similar changes where ushort_converter is used.

"unsigned short");
}
return NULL;
Expand Down Expand Up @@ -462,7 +462,7 @@ poll_modify(pollObject *self, PyObject *args)
assert(PyErr_Occurred());
if (PyErr_ExceptionMatches(PyExc_OverflowError)) {
PyErr_SetString(PyExc_OverflowError,
"modify: eventmask value does not fit in C "
"eventmask value does not fit in C "
"unsigned short");
}
return NULL;
Expand Down Expand Up @@ -808,8 +808,8 @@ internal_devpoll_register(devpollObject *self, PyObject *args, int remove)
assert(PyErr_Occurred());
if (PyErr_ExceptionMatches(PyExc_OverflowError)) {
PyErr_SetString(PyExc_OverflowError,
"register/modify: eventmask value does not fit "
"in C unsigned short");
"eventmask value does not fit in C "
"unsigned short");
}
return NULL;
}
Expand Down
1 change: 1 addition & 0 deletions Objects/bytesobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ byte_converter(PyObject *arg, char *p)
else {
iobj = PyNumber_Index(arg);
if (iobj == NULL) {
assert(PyErr_Occurred());
if (!PyErr_ExceptionMatches(PyExc_TypeError)) {
return 0;
}
Expand Down