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

PyArg_ParseTuple with format "et#" and "es#" detects overflow by raising TypeError instead of ValueError #70386

Closed
hniksic mannequin opened this issue Jan 25, 2016 · 16 comments
Assignees
Labels
docs Documentation in the Doc dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@hniksic
Copy link
Mannequin

hniksic mannequin commented Jan 25, 2016

BPO 26198
Nosy @hniksic, @berkerpeksag, @vadmium, @serhiy-storchaka
Files
  • typeerror.patch
  • pyarg_parse_encoded_string.patch
  • pyarg_parse_encoded_string_2.patch
  • 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 2016-02-08.18:53:30.874>
    created_at = <Date 2016-01-25.16:13:08.267>
    labels = ['interpreter-core', 'type-bug', 'docs']
    title = 'PyArg_ParseTuple with format "et#" and "es#" detects overflow by raising TypeError instead of ValueError'
    updated_at = <Date 2016-02-08.18:53:30.873>
    user = 'https://github.com/hniksic'

    bugs.python.org fields:

    activity = <Date 2016-02-08.18:53:30.873>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2016-02-08.18:53:30.874>
    closer = 'serhiy.storchaka'
    components = ['Documentation', 'Interpreter Core']
    creation = <Date 2016-01-25.16:13:08.267>
    creator = 'hniksic'
    dependencies = []
    files = ['41712', '41720', '41746']
    hgrepos = []
    issue_num = 26198
    keywords = ['patch']
    message_count = 16.0
    messages = ['258905', '258907', '258915', '258917', '258957', '258960', '259151', '259163', '259167', '259170', '259171', '259172', '259809', '259820', '259825', '259826']
    nosy_count = 7.0
    nosy_names = ['hniksic', 'docs@python', 'python-dev', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'Barun Parruck']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26198'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented Jan 25, 2016

    The documentation for the "es#" format (and the "et#" that derives from it) documents the possibility of providing an already allocated buffer. Buffer overflow is detected and handled as follows: "If the buffer is not large enough, a ValueError will be set."

    However, the actual behavior is to raise a TypeError. Inspecting the code in getargs.c reveals that convertsimple() handles buffer overflow by returning a formatted message to its caller, convertitem(). Calls to convertitem() that return an error call seterror() to set the error, and seterror() unconditionally sets the PyExc_TypeError.

    This is not a big issue in practice, and since the behavior is not new, it might be best to simply update the documentation to match the existing practice instead of changing the behavior and risking breaking working code.

    @hniksic hniksic mannequin assigned docspython Jan 25, 2016
    @hniksic hniksic mannequin added docs Documentation in the Doc dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Jan 25, 2016
    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented Jan 25, 2016

    The problem can be encountered and easily reproduced by calling os.path functions, such as os.path.abspath, with a sufficiently large string on Windows:

    >>> os.path.abspath("a" * 1024)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "P:\...\lib\ntpath.py", line 471, in abspath
    TypeError: must be (buffer overflow), not str

    The error message is somewhat confusing, making it look like the "must be" and "not" arguments are swapped. Ideally, the message could be along the lines of "must be a string of no more than X characters".

    @BarunParruck
    Copy link
    Mannequin

    BarunParruck mannequin commented Jan 25, 2016

    I just changed the ValueError to TypeError. This is my first attempt at fixing anything, so let me know if I've screwed up anywhere.

    @BarunParruck
    Copy link
    Mannequin

    BarunParruck mannequin commented Jan 25, 2016

    Added a patch that changes the documentation to reflect TypeError instead of ValueError*

    @serhiy-storchaka
    Copy link
    Member

    Proposed patch fixes minor bugs in Python/getargs.c:

    1. Replaces TypeError with confusing error message for buffer overflow for "es#" and "et#" format units to ValueError with correct error message. Due to the risk to break working code, may be we will left TypeError in maintained releases, but error message should be fixed in any case.

    2. Replaces all other TypeError with confusing error message to SystemError with correct error message. All this errors are programming errors (incorrect use of PyArg_Parse* functions) and aren't occurred in valid extensions.

    3. Fixes error messages for "k" and "K" format units.

    4. Fixes error messages for "es" and "et" format units.

    5. Fixes error messages for "compat" mode (looks as this mode is not used and can be freely removed in Python 3).

    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented Jan 26, 2016

    Barun, Serhiy, thanks for picking this up so quickly.

    I would further suggest to avoid using a fixed buffer in abspath (_getfullpathname, but only abspath seems to call it). Other filesystem calls are using the interface where PyArg_ParseTuple allocates the buffer. This makes it easier to handling errors from the native layer by catching OSError or similar, instead of the very generic TypeError (or ValueError).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 28, 2016

    New changeset 1c690cb4def8 by Serhiy Storchaka in branch '3.5':
    Issue bpo-26198: Added tests for "es", "et", "es#", "et#" and "C" format units
    https://hg.python.org/cpython/rev/1c690cb4def8

    New changeset 745ad3010384 by Serhiy Storchaka in branch 'default':
    Issue bpo-26198: Added tests for "es", "et", "es#", "et#" and "C" format units
    https://hg.python.org/cpython/rev/745ad3010384

    New changeset 60a2d67dacb3 by Serhiy Storchaka in branch '2.7':
    Issue bpo-26198: Added tests for string-related format units of PyArg_Parse*()
    https://hg.python.org/cpython/rev/60a2d67dacb3

    @vadmium
    Copy link
    Member

    vadmium commented Jan 28, 2016

    One review comment

    @vadmium
    Copy link
    Member

    vadmium commented Jan 28, 2016

    Also, test_getargs2 seems faulty on 2.7. I am seeing MemoryError; some of the buildbots are segfaulting.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 28, 2016

    New changeset d0b4be7d2134 by Serhiy Storchaka in branch '2.7':
    Fixed a crash in new tests in test_getargs2 added in 60a2d67dacb3 (issue bpo-26198).
    https://hg.python.org/cpython/rev/d0b4be7d2134

    @serhiy-storchaka
    Copy link
    Member

    Thank you Martin. This was 64-bit specific bug.

    @serhiy-storchaka
    Copy link
    Member

    Here is updated patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 7, 2016

    New changeset ab25ce400abd by Serhiy Storchaka in branch '2.7':
    Issue bpo-26198: Fixed error messages for some argument parsing errors.
    https://hg.python.org/cpython/rev/ab25ce400abd

    New changeset 9f998e24d8d8 by Serhiy Storchaka in branch '3.5':
    Issue bpo-26198: Fixed error messages for some argument parsing errors.
    https://hg.python.org/cpython/rev/9f998e24d8d8

    New changeset f8bdc0ea3bcf by Serhiy Storchaka in branch 'default':
    Issue bpo-26198: Fixed error messages for some argument parsing errors.
    https://hg.python.org/cpython/rev/f8bdc0ea3bcf

    New changeset 53d66a554beb by Serhiy Storchaka in branch 'default':
    Issue bpo-26198: ValueError is now raised instead of TypeError on buffer
    https://hg.python.org/cpython/rev/53d66a554beb

    @berkerpeksag
    Copy link
    Member

    test_datetime is broken on 3.5 and default:

    ======================================================================
    FAIL: test_format (test.datetimetester.TestSubclassDateTime)
    ----------------------------------------------------------------------
    TypeError: __format__() argument 1 must be str, not int

    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "/var/lib/buildbot/slaves/profile-opt-bot/3.5.gps-debian-profile-opt/build/Lib/test/datetimetester.py", line 1578, in test_format
        dt.__format__(123)
    AssertionError: "^must be str, not int$" does not match "__format__() argument 1 must be str, not int"

    http://buildbot.python.org/all/builders/AMD64%20Debian%20PGO%203.5/builds/632/steps/test/logs/stdio

    http://buildbot.python.org/all/builders/x86%20Tiger%203.x/builds/10486/steps/test/logs/stdio

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 8, 2016

    New changeset 22b0a63808f8 by Serhiy Storchaka in branch '3.5':
    Issue bpo-26198: Make datetime error tests more lenient.
    https://hg.python.org/cpython/rev/22b0a63808f8

    New changeset a9c9e4054f3f by Serhiy Storchaka in branch 'default':
    Issue bpo-26198: Make datetime error tests more lenient.
    https://hg.python.org/cpython/rev/a9c9e4054f3f

    @serhiy-storchaka
    Copy link
    Member

    Thanks Berker.

    @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
    docs Documentation in the Doc dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants