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

[hniksic]

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]

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]

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]

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]

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

[serhiy-storchaka]

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]

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]

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]

One review comment

[vadmium]

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