Visitar URL original
gh-101283: Try to load the fallback cmd.exe by an absolute path by arhadthedev · Pull Request #101286 · python/cpython · GitHub
Skip to content

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.

Sign up for GitHub

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

Merged
zooba merged 25 commits into from
Feb 8, 2023
Merged

gh-101283: Try to load the fallback cmd.exe by an absolute path #101286

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d122f14
Try to load the fallback cmd.exe by an absolute path
arhadthedev Jan 24, 2023
e96457c
Add a NEWS entry
arhadthedev Jan 24, 2023
b26918e
Fix minor grammar
arhadthedev Jan 24, 2023
9a0fdbd
Address patchcheck CI error report
arhadthedev Jan 24, 2023
091e970
Address the review
arhadthedev Jan 24, 2023
a3cbdb6
Simplify NEWS wording and reattribute the totally rewritten PR
arhadthedev Jan 24, 2023
8dab7f1
Address the review-2
arhadthedev Jan 24, 2023
1d8d4a6
Update Lib/subprocess.py
arhadthedev Jan 24, 2023
9ed1758
Simplify the NEWS entry
arhadthedev Jan 25, 2023
a2734a6
Clarify attribution
arhadthedev Jan 25, 2023
407aa35
Update 2023-01-24-16-12-00.gh-issue-101283.9tqu39.rst
arhadthedev Jan 25, 2023
f1dfcff
Merge branch 'main' into absolute-cmdexe-fallback-patch
arhadthedev Jan 28, 2023
880d443
Add `Changed in version` entries
arhadthedev Jan 28, 2023
b34f998
Try another multiversion syntax
arhadthedev Jan 28, 2023
84cf94f
Try the third, multiline syntax
arhadthedev Jan 28, 2023
9cbdfca
Apply the gpshead's suggestion
arhadthedev Jan 30, 2023
8bfb11b
Merge branch 'main' into absolute-cmdexe-fallback-patch
arhadthedev Jan 31, 2023
86473da
Address the review
arhadthedev Feb 8, 2023
f1b4412
Apply suggestions from code review
arhadthedev Feb 8, 2023
11ebd69
Improve performance by triggering `cmd.exe` search on `executable=Non…
arhadthedev Feb 8, 2023
25dcbb8
Update the Changed In section
arhadthedev Feb 8, 2023
fc08548
Update Lib/subprocess.py
arhadthedev Feb 8, 2023
7e06703
Fix env variable name misquoting
arhadthedev Feb 8, 2023
81aba86
Manually fix env variable name misquoting
arhadthedev Feb 8, 2023
fecd606
Update Lib/subprocess.py
arhadthedev Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion Lib/subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1480,7 +1480,23 @@ 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: with no full path, Windows looks into a
# current directory first so no plain "cmd.exe".
Comment thread
arhadthedev marked this conversation as resolved.
Outdated
comspec = os.environ.get('ComSpec')
Comment thread
eryksun marked this conversation as resolved.
Outdated
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')
Comment thread
arhadthedev marked this conversation as resolved.
Outdated
if not os.path.isfile(comspec):
Comment thread
zooba marked this conversation as resolved.
Outdated
Comment thread
arhadthedev marked this conversation as resolved.
Outdated
# 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.

@gpshead gpshead Jan 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?

@eryksun eryksun Jan 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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)

@gpshead gpshead Jan 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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):
Comment thread
arhadthedev marked this conversation as resolved.
Outdated
executable = comspec

args = '{} /c "{}"'.format (comspec, args)

if cwd is not None:
Expand Down
6 changes: 6 additions & 0 deletions Misc/NEWS.d/next/Security/2023-01-24-16-12-00.gh-issue-101283.9tqu39.rst
View file
Open in desktop
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
Comment thread
arhadthedev marked this conversation as resolved.
Outdated
are totally stripped and Windows is installed not into ``C\Windows``.
Comment thread
arhadthedev marked this conversation as resolved.
Outdated
Patch by Eryk Sun.
Comment thread
eryksun marked this conversation as resolved.
Outdated

@zooba zooba Jan 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.