[issue26198] PyArg_ParseTuple with format "et#" and "es#" detects overflow by raising TypeError instead of ValueError
New submission from Hrvoje Nikšić: 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. ---------- assignee: docs@python components: Documentation, Interpreter Core messages: 258905 nosy: docs@python, hniksic priority: normal severity: normal status: open title: PyArg_ParseTuple with format "et#" and "es#" detects overflow by raising TypeError instead of ValueError type: behavior versions: Python 2.7, Python 3.5 _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Hrvoje Nikšić added the comment: 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". ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Barun Parruck added the comment: 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. ---------- keywords: +patch nosy: +Barun Parruck Added file: http://bugs.python.org/file41712/typeerror.patch _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Barun Parruck added the comment: Added a patch that changes the documentation to reflect TypeError instead of ValueError* ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Serhiy Storchaka added the comment: 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). ---------- assignee: docs@python -> serhiy.storchaka nosy: +serhiy.storchaka stage: -> patch review versions: +Python 3.6 Added file: http://bugs.python.org/file41720/pyarg_parse_encoded_string.patch _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Hrvoje Nikšić added the comment: 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 tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Roundup Robot added the comment: 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 ---------- nosy: +python-dev _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Martin Panter added the comment: One review comment ---------- nosy: +martin.panter _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Martin Panter added the comment: Also, test_getargs2 seems faulty on 2.7. I am seeing MemoryError; some of the buildbots are segfaulting. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Roundup Robot added the comment: 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 ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Serhiy Storchaka added the comment: Thank you Martin. This was 64-bit specific bug. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Serhiy Storchaka added the comment: Here is updated patch. ---------- Added file: http://bugs.python.org/file41746/pyarg_parse_encoded_string_2.patch _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Roundup Robot added the comment: 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 ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Berker Peksag added the comment: 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/63... http://buildbot.python.org/all/builders/x86%20Tiger%203.x/builds/10486/steps... ---------- nosy: +berker.peksag _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Roundup Robot added the comment: 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 ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Serhiy Storchaka added the comment: Thanks Berker. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
Changes by Serhiy Storchaka <storchaka@gmail.com>: ---------- resolution: -> fixed stage: patch review -> resolved status: open -> closed _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue26198> _______________________________________
participants (6)
-
Barun Parruck -
Berker Peksag
-
Hrvoje Nikšić
-
Martin Panter
-
Roundup Robot
-
Serhiy Storchaka