-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-101283: Try to load the fallback cmd.exe by an absolute path #101286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
d122f14
e96457c
b26918e
9a0fdbd
091e970
a3cbdb6
8dab7f1
1d8d4a6
9ed1758
a2734a6
407aa35
f1dfcff
880d443
b34f998
84cf94f
9cbdfca
8bfb11b
86473da
f1b4412
11ebd69
25dcbb8
fc08548
7e06703
81aba86
fecd606
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1480,7 +1480,26 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, | |
if shell: | ||
startupinfo.dwFlags |= _winapi.STARTF_USESHOWWINDOW | ||
startupinfo.wShowWindow = _winapi.SW_HIDE | ||
comspec = os.environ.get("COMSPEC", "cmd.exe") | ||
# gh-101283: without a fully-qualified path, before Windows | ||
# checks the system directories, it first looks in the | ||
# application directory, and also the current directory if | ||
# NeedCurrentDirectoryForExePathW(ExeName) is true, so try | ||
# to avoid executing unqualified "cmd.exe". | ||
comspec = os.environ.get('ComSpec') | ||
if not comspec: | ||
system_drive = os.environ.get('SystemDrive') or 'C:' | ||
system_root = os.environ.get('SystemRoot') or os.path.join( | ||
system_drive, 'Windows') | ||
comspec = os.path.join(system_root, 'System32', 'cmd.exe') | ||
arhadthedev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if not os.path.isfile(comspec): | ||
zooba marked this conversation as resolved.
Show resolved
Hide resolved
arhadthedev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# cmd.exe is missing, or the system environment | ||
# variables are broken, or they're undefined and the | ||
# system is installed into a non-standard location. | ||
# This is highly unlikely, and we cannot help here. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should a RuntimeWarning be raised or logging.warn call made in this case as an FYI? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good idea. How about this? comspec = 'cmd.exe'
warnings.warn(f'spawning "{comspec}" using a '
'relative file path',
RuntimeWarning) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we keep the fallback at all rather than just make it an error, something like that. probably with a stacklevel= set. related to my other comment, i wouldn't want to add a new warning in a security backport either. |
||
comspec = 'cmd.exe' | ||
if not executable and os.path.isabs(comspec): | ||
arhadthedev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
executable = comspec | ||
|
||
args = '{} /c "{}"'.format (comspec, args) | ||
|
||
if cwd is not None: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
Windows version of :meth:`subprocess.Popen.__init__` with | ||
``shell=True`` made its shell search algorithm protected from dropping | ||
a program named ``cmd.exe`` into a current directory. Now the current | ||
directory is used as the last resort if system environment variables | ||
arhadthedev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
are totally stripped and Windows is installed not into ``C\Windows``. | ||
arhadthedev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Patch by Eryk Sun. | ||
eryksun marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the kind of too-much-detail I was concerned about in my review comments. Let's keep the NEWS entry simple and update the docs, but make sure we retain the ability to use an even more reliable algorithm in the future. |
Uh oh!
There was an error while loading. Please reload this page.