classification
Title: PyArg_ParseTuple with format "et#" and "es#" detects overflow by raising TypeError instead of ValueError
Type: behavior Stage: resolved
Components: Documentation, Interpreter Core Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Barun Parruck, berker.peksag, docs@python, hniksic, martin.panter, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-01-25 16:13 by hniksic, last changed 2016-02-08 18:53 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
typeerror.patch Barun Parruck, 2016-01-25 18:10 review
pyarg_parse_encoded_string.patch serhiy.storchaka, 2016-01-26 14:08 review
pyarg_parse_encoded_string_2.patch serhiy.storchaka, 2016-01-28 23:08 review
Messages (16)
msg258905 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2016-01-25 16:13
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.
msg258907 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2016-01-25 16:24
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".
msg258915 - (view) Author: Barun Parruck (Barun Parruck) * Date: 2016-01-25 18:10
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.
msg258917 - (view) Author: Barun Parruck (Barun Parruck) * Date: 2016-01-25 19:05
Added a patch that changes the documentation to reflect TypeError instead of ValueError*
msg258957 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-01-26 14:08
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).
msg258960 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2016-01-26 14:29
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).
msg259151 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-01-28 17:58
New changeset 1c690cb4def8 by Serhiy Storchaka in branch '3.5':
Issue #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 #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 #26198: Added tests for string-related format units of PyArg_Parse*()
https://hg.python.org/cpython/rev/60a2d67dacb3
msg259163 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-01-28 21:53
One review comment
msg259167 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-01-28 22:36
Also, test_getargs2 seems faulty on 2.7. I am seeing MemoryError; some of the buildbots are segfaulting.
msg259170 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-01-28 22:56
New changeset d0b4be7d2134 by Serhiy Storchaka in branch '2.7':
Fixed a crash in new tests in test_getargs2 added in 60a2d67dacb3 (issue #26198).
https://hg.python.org/cpython/rev/d0b4be7d2134
msg259171 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-01-28 23:03
Thank you Martin. This was 64-bit specific bug.
msg259172 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-01-28 23:08
Here is updated patch.
msg259809 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-02-07 23:23
New changeset ab25ce400abd by Serhiy Storchaka in branch '2.7':
Issue #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 #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 #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 #26198: ValueError is now raised instead of TypeError on buffer
https://hg.python.org/cpython/rev/53d66a554beb
msg259820 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-02-08 05:55
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
msg259825 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-02-08 07:27
New changeset 22b0a63808f8 by Serhiy Storchaka in branch '3.5':
Issue #26198: Make datetime error tests more lenient.
https://hg.python.org/cpython/rev/22b0a63808f8

New changeset a9c9e4054f3f by Serhiy Storchaka in branch 'default':
Issue #26198: Make datetime error tests more lenient.
https://hg.python.org/cpython/rev/a9c9e4054f3f
msg259826 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-02-08 07:30
Thanks Berker.
History
Date User Action Args
2016-02-08 18:53:30serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-02-08 07:30:47serhiy.storchakasetmessages: + msg259826
2016-02-08 07:27:50python-devsetmessages: + msg259825
2016-02-08 05:55:56berker.peksagsetnosy: + berker.peksag
messages: + msg259820
2016-02-07 23:23:16python-devsetmessages: + msg259809
2016-01-28 23:08:09serhiy.storchakasetfiles: + pyarg_parse_encoded_string_2.patch

messages: + msg259172
2016-01-28 23:03:04serhiy.storchakasetmessages: + msg259171
2016-01-28 22:56:45python-devsetmessages: + msg259170
2016-01-28 22:36:37martin.pantersetmessages: + msg259167
2016-01-28 21:53:46martin.pantersetnosy: + martin.panter
messages: + msg259163
2016-01-28 17:58:01python-devsetnosy: + python-dev
messages: + msg259151
2016-01-26 14:29:22hniksicsetmessages: + msg258960
2016-01-26 14:08:30serhiy.storchakasetfiles: + pyarg_parse_encoded_string.patch

assignee: docs@python -> serhiy.storchaka
versions: + Python 3.6
nosy: + serhiy.storchaka

messages: + msg258957
stage: patch review
2016-01-25 19:05:45Barun Parrucksetmessages: + msg258917
2016-01-25 18:10:19Barun Parrucksetfiles: + typeerror.patch

nosy: + Barun Parruck
messages: + msg258915

keywords: + patch
2016-01-25 16:24:48hniksicsetmessages: + msg258907
2016-01-25 16:13:08hniksiccreate