Skip to content

bpo-35755: Remove current directory from posixpath.defpath#11586

Merged
vstinner merged 1 commit intopython:masterfrom
vstinner:defpath
Apr 17, 2019
Merged

bpo-35755: Remove current directory from posixpath.defpath#11586
vstinner merged 1 commit intopython:masterfrom
vstinner:defpath

Conversation

@vstinner
Copy link
Copy Markdown
Member

@vstinner vstinner commented Jan 17, 2019 •
edited
Loading

Document the change in a NEWS entry of the Security category.

https://bugs.python.org/issue35755

@vstinner vstinner added the type-security A security issue label Jan 17, 2019
@vstinner vstinner requested a review from gpshead as a code owner January 17, 2019 09:25
@vstinner vstinner changed the title bpo-35755: Remove current directory from os.defpath [WIP] bpo-35755: Remove current directory from os.defpath Jan 17, 2019
@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Jan 17, 2019

I would prefer to not merge this change before we decided how PATH="" (empty string) should be handled: https://bugs.python.org/issue35755#msg333832

asottile
asottile reviewed Jan 17, 2019
View reviewed changes
asottile
asottile reviewed Jan 17, 2019
View reviewed changes
@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Jan 17, 2019

Hum. The test fails on Windows:

FAIL: test_defpath (test.test_subprocess.ProcessTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\cpython\lib\test\test_subprocess.py", line 1402, in test_defpath
    subprocess.run(args, env=env)
AssertionError: FileNotFoundError not raised

vstinner
vstinner commented Jan 17, 2019
View reviewed changes
Lib/test/test_subprocess.py Outdated
Copy link
Copy Markdown
Member Author

@vstinner vstinner Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. It's now useless, I forgot to remove it. I will remove it once I understand the Windows failure.

@asottile
Copy link
Copy Markdown
Contributor

asottile commented Jan 17, 2019

Hum. The test fails on Windows:

FAIL: test_defpath (test.test_subprocess.ProcessTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\cpython\lib\test\test_subprocess.py", line 1402, in test_defpath
    subprocess.run(args, env=env)
AssertionError: FileNotFoundError not raised

iirc the behaviour you're seeing is due to https://bugs.python.org/issue8557

@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Apr 16, 2019

I wrote PR #12858 to define the behavior when the PATH environment variable is set to an empty string.

@vstinner
bpo-35755: Remove current directory from posixpath.defpath
2da5318 
Document the change in a NEWS entry of the Security category.
@vstinner vstinner changed the title [WIP] bpo-35755: Remove current directory from os.defpath bpo-35755: Remove current directory from posixpath.defpath Apr 17, 2019
@vstinner vstinner merged commit 2c4c02f into python:master Apr 17, 2019
@vstinner vstinner deleted the defpath branch April 17, 2019 15:05
@vstinner vstinner mentioned this pull request Apr 17, 2019
Merged
vstinner added a commit that referenced this pull request Apr 17, 2019
@vstinner
[3.7] bpo-35755: shutil.which() uses os.confstr("CS_PATH") (GH-12862)
394b991 
* bpo-35755: shutil.which() uses os.confstr("CS_PATH") (GH-12858)

shutil.which() and distutils.spawn.find_executable() now use
os.confstr("CS_PATH") if available instead of os.defpath, if the PATH
environment variable is not set.

Don't use os.confstr("CS_PATH") nor os.defpath if the PATH
environment variable is set to an empty string.

Changes:

* find_executable() now starts by checking for the executable in the
  current working directly case. Add an explicit
  "if not path: return None".
* Add tests for PATH='' (empty string), PATH=':' and for PATHEXT.

(cherry picked from commit 228a3c9)

* bpo-35755: Remove current directory from posixpath.defpath (GH-11586)

Document the change in a NEWS entry of the Security category.

(cherry picked from commit 2c4c02f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead Awaiting requested review from gpshead

1 more reviewer

@asottile asottile asottile left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type-security A security issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vstinner @asottile @the-knights-who-say-ni @bedevere-bot