classification
Title: wrong error messages when using PyArg_ParseTuple to parse normal tuples
Type: behavior Stage: resolved
Components: IO Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Oren Milman, rhettinger, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2016-09-23 20:16 by Oren Milman, last changed 2017-08-26 19:01 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
testBugsOrPatches.py Oren Milman, 2016-09-23 20:16 an ugly script that verifies the bugs or the patches
issue28261_ver1.diff Oren Milman, 2016-09-23 20:18 proposed patches diff file - ver1 review
patchedCPythonTestOutput_ver1.txt Oren Milman, 2016-09-23 20:18 test output of CPython with my patches (tested on my PC) - ver1
CPythonTestOutput.txt Oren Milman, 2016-09-23 20:19 test output of CPython without my patches (tested on my PC)
Pull Requests
URL Status Linked Edit
PR 3119 merged Oren Milman, 2017-08-17 14:56
PR 3198 merged Oren Milman, 2017-08-24 14:31
PR 3210 merged Oren Milman, 2017-08-26 09:51
PR 3213 merged Oren Milman, 2017-08-26 17:35
Messages (22)
msg277296 - (view) Author: Oren Milman (Oren Milman) * Date: 2016-09-23 20:16
------------ current state ------------
In few places in the codebase, PyArg_ParseTuple is called in order to parse a normal tuple (instead of the usual case of parsing the arguments tuple of a function).
In some of these places, failure of PyArg_ParseTuple is handled simply by cleanup (if needed) and returning an error code, and so an exception with the generic error message is raised.
The generic error message is appropriate in case the parsed tuple is the arguments tuple of a function, but might be really wrong in case it is a normal tuple.

For example, such case in Modules/socketmodule.c in socket_getnameinfo leads to the following:
    >>> import socket
    >>> socket.getnameinfo(tuple(), 1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: function takes at least 2 arguments (0 given)
    >>>


------------ proposed changes ------------
In each of these places, add to the format string (which is passed to PyArg_ParseTuple) a ';', followed by an appropriate error message, as already done in Modules/audioop.c in audioop_ratecv_impl:
        if (!PyArg_ParseTuple(state,
                        "iO!;audioop.ratecv: illegal state argument",
                        &d, &PyTuple_Type, &samps))
            goto exit;


------------ diff ------------
The proposed patches diff file is attached.


------------ tests ------------
I wrote an ugly script to verify the wrong error messages on CPython without my patches, and to test the patches on CPython with my patches. The script is attached (but it might fail on a non-Windows machine).

In addition, I ran 'python_d.exe -m test -j3' (on my 64-bit Windows 10) with and without the patches, and got quite the same output.
The outputs of both runs are attached.
msg288792 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-03-02 10:05
I dislike passing a error message in the function name. I suggest to instead raise a new exception with a better error message and chain it with the previous exception.
msg288861 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-03 06:50
Do you mean that in each case PyArg_ParseTuple fails, we should chain
to the exception raised by PyArg_ParseTuple an exception that specifies
the name of the tuple that PyArg_ParseTuple failed to parse, without
specifying the function name?
msg288862 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-03 07:01
An error message is not passed in the function name. PyArg_ParseTuple() allows you to pass an arbitrary error message. I think this feature is specially designed for these cases.

I didn't look the patch close, but Oren's approach LGTM in general.
msg288869 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-03 07:39
Oren, could you write reproducers for all affected cases? I don't think we need to add them as regular tests, but we should be able to check changes manually.

There are conflicts in Modules/itertoolsmodule.c.
msg288911 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-03-03 18:56
-0 The new code looks awful, and in the case of the itertools module, these are messages that users are unlikely to see (the methods are typically only called by pickle, and pickling itself is rare for itertools).
msg288917 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-03 19:18
Would it look less awful if remove "xxxxxx.__setstate__: "?
msg289672 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-15 13:00
Raymond? any suggestions for how to make the code less ugly?
or do you have in mind a different approach for solving this issue?
msg289673 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-15 13:06
I suggest to withdraw changes in itertoolsmodule.c and avoid using "dunder" names like __setstate__ and __new__ in error messages.
msg289727 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-16 18:30
as Serhiy pointed out in PR 668, here are some more functions that produce the
same kind of wrong error messages:
    - Modules/timemodule.c - gettmarg()
    - Modules/socketmodule.c:
        * getsockaddrarg()
        * socket_getnameinfo()

ISTM that searching for 'PyTuple_Check(' might be a good way to find more such
functions, as they sometimes raise an error in case a type other than tuple was
received (and after that, PyArg_ParseTuple is called).
msg300366 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-08-16 14:26
I replied to your comments in Rietveld, Serhiy. (http://bugs.python.org/review/28261)

also, i found two places with a quite similar issue:
- in Objects/exceptions.c in ImportError_init:
    >>> ImportError(1, 2, 3, 4, a=5, b=6, c=7)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: ImportError() takes at most 2 arguments (3 given)
- in Python/bltinmodule.c in min_max:
    >>> min(1, 2, 3, 4, a=5, b=6, c=7)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: function takes at most 2 arguments (3 given)

may I fix them also as part of this issue?
msg300402 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-08-17 07:59
After more looking, I found this issue in two more places:
- in Modules/itertoolsmodule.c in product_new:
    >>> itertools.product(0, a=1, b=2, c=3, d=4, e=5, f=6)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: product() takes at most 1 argument (6 given)
- in Python/bltinmodule.c in builtin_print: 
    >>> print(0, a=1, b=2, c=3, d=4, e=5, f=6)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: print() takes at most 4 arguments (6 given)

what do you think?
should I open another issue for these and the other two I
mentioned in msg300366?
msg300417 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-08-17 12:52
> also, i found two places with a quite similar issue:
> may I fix them also as part of this issue?

This looks as a different issue to me. All these cases are similar, but different from the original issue. Please open a new issue.
msg300418 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-08-17 13:05
Answered questions on Rietveld. I'm not sure email notification from Rietveld works now.
msg300604 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-08-20 15:35
New changeset 1d1d3e9db882d78433f5bc8dbe7df929f4b6b5e1 by Serhiy Storchaka (Oren Milman) in branch 'master':
bpo-28261: Fixed err msgs where PyArg_ParseTuple is used to parse normal tuples. (#3119)
https://github.com/python/cpython/commit/1d1d3e9db882d78433f5bc8dbe7df929f4b6b5e1
msg300614 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-08-21 08:38
it seems that I have missed some places which are part of this issue, at least
in Modules/_io/textio.c (one of them is mentioned in #31243).

also, when fixing these, we should also add a check before the call to
PyArg_ParseTuple (in case such check doesn't already exist), to verify the
object is indeed a tuple.
msg300792 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-08-24 16:51
New changeset 13614e375cc3637cf1311733d453df6107e964ea by Serhiy Storchaka (Oren Milman) in branch 'master':
bpo-28261: fix err msgs where PyArg_ParseTuple is used to parse normal tuples (leftovers) (#3198)
https://github.com/python/cpython/commit/13614e375cc3637cf1311733d453df6107e964ea
msg300841 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-08-25 14:26
Oren, do you mind to backport the part of your changes that adds explicit checks for tuples to 3.6 and 2.7? Raising SystemError looks like a bug to me.
msg300843 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-08-25 14:36
sure
msg300875 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-08-26 12:27
New changeset 8e67981fc8e1bf3cb9774b5fbf4a39b8d65ba4ff by Serhiy Storchaka (Oren Milman) in branch '3.6':
[3.6] bpo-28261: Prevent raising SystemError where PyArg_ParseTuple is used to parse non-args. (#3210)
https://github.com/python/cpython/commit/8e67981fc8e1bf3cb9774b5fbf4a39b8d65ba4ff
msg300893 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-08-26 18:56
New changeset bc80fd1bd2e8b6817accbc101e7fe5e50ba8f768 by Serhiy Storchaka (Oren Milman) in branch '2.7':
[2.7] bpo-28261: Prevent raising SystemError where PyArg_ParseTuple is used to parse non-args. (#3213)
https://github.com/python/cpython/commit/bc80fd1bd2e8b6817accbc101e7fe5e50ba8f768
msg300896 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-08-26 19:01
Thank you for your contribution Oren!
History
Date User Action Args
2017-08-26 19:01:58serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg300896

stage: patch review -> resolved
2017-08-26 18:56:33serhiy.storchakasetmessages: + msg300893
2017-08-26 17:35:54Oren Milmansetpull_requests: + pull_request3251
2017-08-26 12:27:55serhiy.storchakasetmessages: + msg300875
2017-08-26 09:51:40Oren Milmansetpull_requests: + pull_request3248
2017-08-25 14:36:18Oren Milmansetmessages: + msg300843
2017-08-25 14:26:37serhiy.storchakasetmessages: + msg300841
2017-08-24 16:51:26serhiy.storchakasetmessages: + msg300792
2017-08-24 14:31:18Oren Milmansetpull_requests: + pull_request3237
2017-08-21 08:38:12Oren Milmansetmessages: + msg300614
2017-08-20 15:35:38serhiy.storchakasetmessages: + msg300604
2017-08-17 14:56:16Oren Milmansetpull_requests: + pull_request3157
2017-08-17 13:05:12serhiy.storchakasetmessages: + msg300418
2017-08-17 12:52:11serhiy.storchakasetmessages: + msg300417
2017-08-17 07:59:55Oren Milmansetmessages: + msg300402
2017-08-16 14:26:28Oren Milmansetmessages: + msg300366
2017-03-16 18:30:12Oren Milmansetmessages: + msg289727
2017-03-15 13:06:57serhiy.storchakasetmessages: + msg289673
2017-03-15 13:00:47Oren Milmansetmessages: + msg289672
2017-03-03 19:18:57serhiy.storchakasetmessages: + msg288917
2017-03-03 18:56:03rhettingersetnosy: + rhettinger
messages: + msg288911
2017-03-03 07:39:11serhiy.storchakasetmessages: + msg288869
2017-03-03 07:01:57serhiy.storchakasetstage: patch review
2017-03-03 07:01:47serhiy.storchakasetassignee: serhiy.storchaka

messages: + msg288862
nosy: + serhiy.storchaka
2017-03-03 06:50:16Oren Milmansetmessages: + msg288861
2017-03-02 10:05:11vstinnersetnosy: + vstinner
messages: + msg288792
2016-09-23 20:19:49Oren Milmansetfiles: + CPythonTestOutput.txt
2016-09-23 20:19:12Oren Milmansetfiles: + patchedCPythonTestOutput_ver1.txt
2016-09-23 20:18:21Oren Milmansetfiles: + issue28261_ver1.diff
keywords: + patch
2016-09-23 20:16:55Oren Milmancreate