classification
Title: Clean up path_converter in posixmodule.c
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: larry, martin.panter, python-dev, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2016-03-30 09:31 by serhiy.storchaka, last changed 2016-04-08 05:49 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
path_converter_cleanup.patch serhiy.storchaka, 2016-03-30 09:31 review
path_converter_cleanup_2.patch serhiy.storchaka, 2016-04-01 10:16 review
path_converter_cleanup_3.patch serhiy.storchaka, 2016-04-06 18:57 review
Messages (13)
msg262657 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-03-30 09:31
path_converter in Modules/posixmodule.c sequentially tries to convert an argument to str, bytes, and int. If previous conversion is failed, it clears the error and tries with type. This can hide some errors (such as MemoryError) and even cause using unexpected conversion.

Proposed patch cleans up path_converter. In addition it avoids copying the content of instances of string subclass.
msg262695 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-03-31 15:08
I approve in principle, but this patch isn't ready.

If we compile on Win32, and allow_fd is on, and they pass in an invalid fd, your patched code will reach line 914 "length = PyBytes_GET_SIZE(bytes);" but bytes will be uninitialized.
msg262734 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-01 10:16
Good catch! Here is updated patch. It fixes also hiding exception in dir_fd converter.
msg262950 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-04-06 17:10
Can you post the updated patch please?
msg262953 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-06 18:57
Here it is.
msg262954 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-04-06 19:01
LGTM.
msg262955 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-06 19:19
New changeset a866f5727b7f by Serhiy Storchaka in branch 'default':
Issue #26671: Enhanced path_converter.
https://hg.python.org/cpython/rev/a866f5727b7f
msg262956 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-06 19:20
Thank you for your review Larry.
msg262959 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-06 19:55
New changeset 8dc144e47252 by Serhiy Storchaka in branch 'default':
Issue #26671: Fixed #ifdef indentation.
https://hg.python.org/cpython/rev/8dc144e47252
msg262961 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-06 20:03
New changeset 4acdb324a430 by Serhiy Storchaka in branch 'default':
Issue #26671: Fixed #ifdef indentation.
https://hg.python.org/cpython/rev/4acdb324a430
msg263005 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-08 05:10
Looks like the tests may need updating for a changed exception message:

http://buildbot.python.org/all/builders/x86%20Ubuntu%20Shared%203.x/builds/12996/steps/test/logs/stdio
======================================================================
FAIL: test_stat (test.test_posix.PosixTester)
----------------------------------------------------------------------
TypeError: stat: path should be string, bytes or integer, not NoneType

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 415, in test_stat
    posix.stat, None)
AssertionError: "can't specify None for path argument" does not match "stat: path should be string, bytes or integer, not NoneType"

======================================================================
FAIL: test_stat_dir_fd (test.test_posix.PosixTester)
----------------------------------------------------------------------
TypeError: argument should be integer or None, not str

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 867, in test_stat_dir_fd
    posix.stat, support.TESTFN, dir_fd=posix.getcwd())
AssertionError: "should be integer, not" does not match "argument should be integer or None, not str"
msg263007 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-08 05:48
New changeset 633bb190fb76 by Serhiy Storchaka in branch 'default':
Issue #26671: Fixed tests for changed error messages.
https://hg.python.org/cpython/rev/633bb190fb76
msg263008 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-08 05:49
Thank you Martin.
History
Date User Action Args
2016-04-08 05:49:25serhiy.storchakasetstatus: open -> closed

messages: + msg263008
2016-04-08 05:48:50python-devsetmessages: + msg263007
2016-04-08 05:10:04martin.pantersetstatus: closed -> open
nosy: + martin.panter
messages: + msg263005

2016-04-06 20:03:07python-devsetmessages: + msg262961
2016-04-06 19:55:52python-devsetmessages: + msg262959
2016-04-06 19:20:25serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg262956

stage: patch review -> resolved
2016-04-06 19:19:14python-devsetnosy: + python-dev
messages: + msg262955
2016-04-06 19:01:31larrysetmessages: + msg262954
2016-04-06 18:57:57serhiy.storchakasetfiles: + path_converter_cleanup_3.patch

messages: + msg262953
2016-04-06 17:10:41larrysetmessages: + msg262950
2016-04-01 12:26:23vstinnersetnosy: + vstinner
2016-04-01 10:16:21serhiy.storchakasetfiles: + path_converter_cleanup_2.patch

messages: + msg262734
2016-03-31 15:08:02larrysetmessages: + msg262695
2016-03-30 09:31:48serhiy.storchakalinkissue26027 dependencies
2016-03-30 09:31:14serhiy.storchakacreate