bpo-28261: fix err msgs where PyArg_ParseTuple is used to parse normal tuples #3119

orenmn · 2017-08-17T14:56:15Z

according to http://bugs.python.org/issue28261, fix some error messages.

without this patch, each of the following calls would have produced a wrong error message (at least on Windows 10):

ctypes.CFUNCTYPE(None)(())
_io.IncrementalNewlineDecoder(42, 42).setstate(())
audioop.ratecv(b'a', 1, 1, 42, 42, (42, ((),)))
audioop.lin2adpcm(b'a', 1, ())
audioop.adpcm2lin(b'a', 1, ())
_overlapped.Overlapped().ConnectEx(42, ()) (also, _overlapped.Overlapped().ConnectEx(42, 43) would have caused a SystemError.)
socket.getnameinfo((), 42)
time.strftime('aoeu', ())
time.asctime(())
time.mktime(())

https://bugs.python.org/issue28261

serhiy-storchaka · 2017-08-17T19:19:29Z

Modules/_io/textio.c

@@ -562,8 +562,11 @@ _io_IncrementalNewlineDecoder_setstate(nldecoder_object *self,
    PyObject *buffer;
    unsigned long long flag;

-    if (!PyArg_ParseTuple(state, "OK", &buffer, &flag))
+    if (!PyArg_ParseTuple(state,
+                          "OK;IncrementalNewlineDecoder.setstate(): illegal "


If state is not a tuple, PyArg_ParseTuple() raises SystemError which means an internal error in the interpreter or extension. Add an explicit check and raise TypeError if state is not a tuple. See lin2adpcm() for example. This comment is applicable to other changes below.

And this error message looks too long to me. The method name (or at least the class name) could be omitted. The context can be derived from a traceback.

serhiy-storchaka · 2017-08-17T20:11:42Z

Modules/_ctypes/_ctypes.c

@@ -3262,7 +3262,8 @@ PyCFuncPtr_FromDll(PyTypeObject *type, PyObject *args, PyObject *kwds)
        /* Here ftuple is a borrowed reference */
        return NULL;

-    if (!PyArg_ParseTuple(ftuple, "O&O", _get_name, &name, &dll)) {
+    if (!PyArg_ParseTuple(ftuple, "O&O;illegal func_spec argument",
+                          _get_name, &name, &dll)) {


In case of multiline condition the opening { should be moved to the separate line for better readability. This is a new requirement of PEP 7.

This comment is applicable to other changes below.

bedevere-bot · 2017-08-17T20:23:15Z

A Python core developer, serhiy-storchaka, has requested some changes be
made to your pull request before we can consider merging it. If you
could please address their requests along with any other requests in
other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment
on this pull request containing the phrase I didn't expect the Spanish Inquisition!
I will then notify serhiy-storchaka along with any other core developers
who have left a review that you're ready for them to take another look
at this pull request.

…les.

orenmn · 2017-08-18T13:57:31Z

Modules/audioop.c

+            channel = PyTuple_GetItem(samps, chan);
+            if (!PyTuple_Check(channel)) {
+                PyErr_SetString(PyExc_TypeError,
+                                "ratecv(): channel must be a tuple");


i am not sure about this error message, as I failed to find the documentation of the state argument.

Maybe just "illegal state argument"? state is an opaque value. It is returned by ratecv() and passed to other invocation of ratecv(). "channel" is nowhere documented.

orenmn · 2017-08-18T14:33:19Z

note that I also added very basic tests to test_audioop.py and to test_io.py, to verify SystemError is not raised anymore.

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-08-18T14:33:20Z

Nobody expects the Spanish Inquisition!

@serhiy-storchaka: please review the changes made to this pull request.

serhiy-storchaka · 2017-08-18T14:29:34Z

Modules/audioop.c

+            channel = PyTuple_GetItem(samps, chan);
+            if (!PyTuple_Check(channel)) {
+                PyErr_SetString(PyExc_TypeError,
+                                "ratecv(): channel must be a tuple");


Maybe just "illegal state argument"? state is an opaque value. It is returned by ratecv() and passed to other invocation of ratecv(). "channel" is nowhere documented.

serhiy-storchaka · 2017-08-18T14:35:59Z

Modules/overlapped.c

@@ -1024,8 +1026,11 @@ Overlapped_ConnectEx(OverlappedObject *self, PyObject *args)
    BOOL ret;
    DWORD err;

-    if (!PyArg_ParseTuple(args, F_HANDLE "O", &ConnectSocket, &AddressObj))
+    if (!PyArg_ParseTuple(args, F_HANDLE "O!:ConnectEx", &ConnectSocket,


Would not it look better if break a line before &ConnectSocket?

serhiy-storchaka · 2017-08-18T15:07:54Z

Modules/timemodule.c

@@ -586,8 +586,11 @@ time_strftime(PyObject *self, PyObject *args)
        if (_PyTime_localtime(tt, &buf) != 0)
            return NULL;
    }
-    else if (!gettmarg(tup, &buf) || !checktm(&buf))
+    else if (!gettmarg(tup, &buf, "iiiiiiiii;strftime(): illegal time tuple "


Wouldn't it look better if break a line before the format argument and before !checktm(&buf)?

serhiy-storchaka · 2017-08-19T04:42:04Z

I think a NEWS.d entry for this PR is not required. But you can add it if you prefer.

…y some of the code

serhiy-storchaka · 2017-08-19T07:12:17Z

Modules/timemodule.c

@@ -780,8 +781,9 @@ time_asctime(PyObject *self, PyObject *args)
        if (_PyTime_localtime(tt, &buf) != 0)
            return NULL;

-    } else if (!gettmarg(tup, &buf, "iiiiiiiii;asctime(): illegal time tuple "
-                                    "argument") || !checktm(&buf))
+    } else if (!gettmarg(tup, &buf,


Yet one nitpick: break a line between "}" and "else" while we are here.

thanks for the review, Serhiy!

…mal tuples. (python#3119)

orenmn added 4 commits August 17, 2017 14:53

passes testBugsOrPatches.py patches tests.

41a0084

changed to using O\!, as Serhiy advised

8d19d62

removed fixes of the too-many-keyword-arguments-wrong-err-msgs issue

be41797

added some curly braces

f1446c1

the-knights-who-say-ni added the CLA signed label Aug 17, 2017

bedevere-bot added the awaiting review label Aug 17, 2017

serhiy-storchaka self-requested a review August 17, 2017 18:32

serhiy-storchaka requested changes Aug 17, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Aug 17, 2017

fix braces according to PEP 7, and add checks whether objects are tup…

978a5d6

…les.

orenmn commented Aug 18, 2017

View reviewed changes

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 18, 2017

serhiy-storchaka reviewed Aug 18, 2017

View reviewed changes

serhiy-storchaka self-assigned this Aug 19, 2017

serhiy-storchaka added the skip news label Aug 19, 2017

dont mention the undocumented 'channel' in error message, and beautif…

d52719e

…y some of the code

serhiy-storchaka approved these changes Aug 19, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 19, 2017

added a line break before else

868e910

serhiy-storchaka approved these changes Aug 20, 2017

View reviewed changes

serhiy-storchaka merged commit 1d1d3e9 into python:master Aug 20, 2017

This was referenced Aug 20, 2017

replace getsockaddrarg with getnameinfo in an error message in socket_getnameinfo #687

Closed

bpo-28261: fix err msgs where PyArg_ParseTuple is used to parse normal tuples (leftovers) #3198

Merged

This was referenced Aug 26, 2017

[3.6] bpo-28261: Prevent raising SystemError where PyArg_ParseTuple is used to parse non-args #3210

Merged

[2.7] bpo-28261: Prevent raising SystemError where PyArg_ParseTuple is used to parse non-args #3213

Merged

Mariatta removed the awaiting merge label Aug 29, 2017

GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017

bpo-28261: Fixed err msgs where PyArg_ParseTuple is used to parse nor…

cb67fc7

…mal tuples. (python#3119)

serhiy-storchaka removed their assignment Dec 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

bpo-28261: fix err msgs where PyArg_ParseTuple is used to parse normal tuples #3119

bpo-28261: fix err msgs where PyArg_ParseTuple is used to parse normal tuples #3119

Uh oh!

orenmn commented Aug 17, 2017 •

edited by bedevere-bot

Loading

Uh oh!

serhiy-storchaka Aug 17, 2017

Uh oh!

serhiy-storchaka Aug 17, 2017

Uh oh!

bedevere-bot commented Aug 17, 2017

Uh oh!

orenmn Aug 18, 2017

Uh oh!

serhiy-storchaka Aug 18, 2017

Uh oh!

orenmn commented Aug 18, 2017

Uh oh!

bedevere-bot commented Aug 18, 2017

Uh oh!

serhiy-storchaka Aug 18, 2017

Uh oh!

serhiy-storchaka Aug 18, 2017

Uh oh!

serhiy-storchaka Aug 18, 2017

Uh oh!

serhiy-storchaka commented Aug 19, 2017

Uh oh!

serhiy-storchaka Aug 19, 2017

Uh oh!

orenmn Aug 19, 2017

Uh oh!

Uh oh!

Uh oh!

bpo-28261: fix err msgs where PyArg_ParseTuple is used to parse normal tuples #3119

bpo-28261: fix err msgs where PyArg_ParseTuple is used to parse normal tuples #3119

Uh oh!

Conversation

orenmn commented Aug 17, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenmn commented Aug 18, 2017

Uh oh!

bedevere-bot commented Aug 18, 2017

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 Aug 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orenmn commented Aug 17, 2017 •

edited by bedevere-bot

Loading