Skip to content
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.

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

wrong error messages when using PyArg_ParseTuple to parse normal tuples #72448

Closed
orenmn mannequin opened this issue Sep 23, 2016 · 22 comments
Closed

wrong error messages when using PyArg_ParseTuple to parse normal tuples #72448

orenmn mannequin opened this issue Sep 23, 2016 · 22 comments
Assignees
Labels
3.7 (EOL) end of life topic-IO type-bug An unexpected behavior, bug, or error

Comments

@orenmn
Copy link
Mannequin

orenmn mannequin commented Sep 23, 2016

BPO 28261
Nosy @rhettinger, @vstinner, @serhiy-storchaka, @orenmn
PRs
  • 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 (leftovers) #3198
  • [3.6] bpo-28261: Prevent raising SystemError where PyArg_ParseTuple is used to parse non-args #3210
  • [2.7] bpo-28261: Prevent raising SystemError where PyArg_ParseTuple is used to parse non-args #3213
  • Files
  • testBugsOrPatches.py: an ugly script that verifies the bugs or the patches
  • issue28261_ver1.diff: proposed patches diff file - ver1
  • patchedCPythonTestOutput_ver1.txt: test output of CPython with my patches (tested on my PC) - ver1
  • CPythonTestOutput.txt: test output of CPython without my patches (tested on my PC)
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2017-08-26.19:01:58.457>
    created_at = <Date 2016-09-23.20:16:55.347>
    labels = ['type-bug', '3.7', 'expert-IO']
    title = 'wrong error messages when using PyArg_ParseTuple to parse normal tuples'
    updated_at = <Date 2017-08-26.19:01:58.455>
    user = 'https://github.com/orenmn'

    bugs.python.org fields:

    activity = <Date 2017-08-26.19:01:58.455>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2017-08-26.19:01:58.457>
    closer = 'serhiy.storchaka'
    components = ['IO']
    creation = <Date 2016-09-23.20:16:55.347>
    creator = 'Oren Milman'
    dependencies = []
    files = ['44795', '44796', '44797', '44798']
    hgrepos = []
    issue_num = 28261
    keywords = ['patch']
    message_count = 22.0
    messages = ['277296', '288792', '288861', '288862', '288869', '288911', '288917', '289672', '289673', '289727', '300366', '300402', '300417', '300418', '300604', '300614', '300792', '300841', '300843', '300875', '300893', '300896']
    nosy_count = 4.0
    nosy_names = ['rhettinger', 'vstinner', 'serhiy.storchaka', 'Oren Milman']
    pr_nums = ['3119', '3198', '3210', '3213']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue28261'
    versions = ['Python 3.7']

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 23, 2016

    ------------ 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.

    @orenmn orenmn mannequin added 3.7 (EOL) end of life topic-IO type-bug An unexpected behavior, bug, or error labels Sep 23, 2016
    @vstinner
    Copy link
    Member

    vstinner commented Mar 2, 2017

    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.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 3, 2017

    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?

    @serhiy-storchaka
    Copy link
    Member

    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.

    @serhiy-storchaka serhiy-storchaka self-assigned this Mar 3, 2017
    @serhiy-storchaka
    Copy link
    Member

    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.

    @rhettinger
    Copy link
    Contributor

    -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).

    @serhiy-storchaka
    Copy link
    Member

    Would it look less awful if remove "xxxxxx.__setstate__: "?

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 15, 2017

    Raymond? any suggestions for how to make the code less ugly?
    or do you have in mind a different approach for solving this issue?

    @serhiy-storchaka
    Copy link
    Member

    I suggest to withdraw changes in itertoolsmodule.c and avoid using "dunder" names like __setstate__ and __new__ in error messages.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 16, 2017

    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).

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Aug 16, 2017

    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?

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Aug 17, 2017

    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?

    @serhiy-storchaka
    Copy link
    Member

    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.

    @serhiy-storchaka
    Copy link
    Member

    Answered questions on Rietveld. I'm not sure email notification from Rietveld works now.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 1d1d3e9 by Serhiy Storchaka (Oren Milman) in branch 'master':
    bpo-28261: Fixed err msgs where PyArg_ParseTuple is used to parse normal tuples. (bpo-3119)
    1d1d3e9

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Aug 21, 2017

    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 bpo-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.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 13614e3 by Serhiy Storchaka (Oren Milman) in branch 'master':
    bpo-28261: fix err msgs where PyArg_ParseTuple is used to parse normal tuples (leftovers) (bpo-3198)
    13614e3

    @serhiy-storchaka
    Copy link
    Member

    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.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Aug 25, 2017

    sure

    @serhiy-storchaka
    Copy link
    Member

    New changeset 8e67981 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. (bpo-3210)
    8e67981

    @serhiy-storchaka
    Copy link
    Member

    New changeset bc80fd1 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. (bpo-3213)
    bc80fd1

    @serhiy-storchaka
    Copy link
    Member

    Thank you for your contribution Oren!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life topic-IO type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants