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

arhadthedev · 2023-01-24T12:02:33Z

Issue: Undocumented risky behaviour in subprocess module #101283

arhadthedev · 2023-01-24T12:28:15Z

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\a\cpython\cpython\build\test_python_2036�\test_python_worker_5676�'

Looks like a fluke; restarting.

Lib/subprocess.py

Co-authored-by: Eryk Sun <eryksun@gmail.com>

Lib/subprocess.py

Co-authored-by: Eryk Sun <eryksun@gmail.com>

arhadthedev · 2023-01-24T16:54:30Z

@eryksun Thank you for improvements! Since none of my code is left, I've reattributed the news entry to you.

Lib/subprocess.py

Co-authored-by: Eryk Sun <eryksun@gmail.com>

Lib/subprocess.py

gpshead · 2023-01-24T21:27:38Z

Lib/subprocess.py

+                        # 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.


Should a RuntimeWarning be raised or logging.warn call made in this case as an FYI?

That's a good idea. How about this?

comspec = 'cmd.exe' warnings.warn(f'spawning "{comspec}" using a ' 'relative file path', RuntimeWarning)

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.

Misc/NEWS.d/next/Security/2023-01-24-16-12-00.gh-issue-101283.9tqu39.rst

…ue on Windows (pythonGH-101286) (cherry picked from commit 23751ed) Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>

miss-islington · 2023-02-08T22:13:47Z

Thanks @arhadthedev for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington · 2023-02-08T22:13:48Z

Sorry @arhadthedev and @zooba, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 23751ed826ee63fb486e874ec25934ea87dd8519 3.11

miss-islington · 2023-02-08T22:14:48Z

Thanks @arhadthedev for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

bedevere-bot · 2023-02-08T22:15:00Z

GH-101711 is a backport of this pull request to the 3.11 branch.

…ue on Windows (pythonGH-101286) (cherry picked from commit 23751ed) Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>

zooba · 2023-02-08T22:20:52Z

Bah, just noticed in the backports that now there's a case where comspec may be used without being defined. We should fix and backport that immediately

zooba · 2023-02-08T22:22:36Z

Turns out I can commit directly to the backports, so those will be fine. But we need an else: comspec = executable branch in main

bedevere-bot · 2023-02-08T22:36:45Z

GH-101713 is a backport of this pull request to the 3.7 branch.

…ue on Windows (pythonGH-101286)

zooba · 2023-02-08T22:37:42Z

GH-101712 is the comspec=executable fix for main.

gpshead · 2023-02-08T22:54:09Z

Doc/library/subprocess.rst

@@ -111,6 +111,14 @@ underlying :class:`Popen` interface can be used directly.
      Added the *text* parameter, as a more understandable alias of *universal_newlines*.
      Added the *capture_output* parameter.

+   .. versionchanged:: 3.11.2


This version number is wrong. 3.11.2 was released before this change was merged. As this PR is in the main branch, listing it as 3.12 in these docs is more appropriate. Documentation readers should not be expected to know which patch release versions came before a major release.

We lack the ability to list all versions in versionchanged markers in our docs so have done it this way elsewhere as well when a visible API change has been made in patch releases.

arhadthedev · 2023-02-09T01:48:11Z

Bah, just noticed in the backports that now there's a case where comspec may be used without being defined. We should fix and backport that immediately

This version number is wrong. 3.11.2 was released before this change was merged. As this PR is in the main branch, listing it as 3.12 in these docs is more appropriate. Documentation readers should not be expected to know which patch release versions came before a major release.

Got working on it; the PR will follow in a few hours.

arhadthedev · 2023-02-09T06:11:50Z

Bah, just noticed in the backports that now there's a case where comspec may be used without being defined. We should fix and backport that immediately

To be fixed by gh-101712. Thank you, @zooba!

This version number is wrong. 3.11.2 was released before this change was merged. As this PR is in the main branch, listing it as 3.12 in these docs is more appropriate. Documentation readers should not be expected to know which patch release versions came before a major release.

Addressed in gh-101728.

…ue on Windows (GH-101286) (#101709) Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net> Co-authored-by: Steve Dower <steve.dower@microsoft.com>

…ue on Windows (GH-101286) (#101710) Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net> Co-authored-by: Steve Dower <steve.dower@microsoft.com>

…ue on Windows (GH-101286) (#101713) Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net> Co-authored-by: Łukasz Langa <lukasz@langa.pl> Co-authored-by: Oleg Iarygin <dralife@yandex.ru>

* main: (82 commits) pythongh-101670: typo fix in PyImport_ExtendInittab() (python#101723) pythonGH-99293: Document that `Py_TPFLAGS_VALID_VERSION_TAG` shouldn't be used. (#pythonGH-101736) no-issue: Add Dong-hee Na as the cjkcodecs codeowner (pythongh-101731) pythongh-101678: Merge math_1_to_whatever() and math_1() (python#101730) pythongh-101678: refactor the math module to use special functions from c11 (pythonGH-101679) pythongh-85984: Remove legacy Lib/pty.py code. (python#92365) pythongh-98831: Use opcode metadata for stack_effect() (python#101704) pythongh-101283: Version was just released, so should be changed in 3.11.3 (pythonGH-101719) pythongh-101283: Fix use of unbound variable (pythonGH-101712) pythongh-101283: Improved fallback logic for subprocess with shell=True on Windows (pythonGH-101286) pythongh-101277: Port more itertools static types to heap types (python#101304) pythongh-98831: Modernize CALL and family (python#101508) pythonGH-101696: invalidate type version tag in `_PyStaticType_Dealloc` (python#101697) pythongh-100221: Fix creating dirs in `make sharedinstall` (pythonGH-100329) pythongh-101670: typo fix in PyImport_AppendInittab() (pythonGH-101672) pythongh-101196: Make isdir/isfile/exists faster on Windows (pythonGH-101324) pythongh-101614: Don't treat python3_d.dll as a Python DLL when checking extension modules for incompatibility (pythonGH-101615) pythongh-100933: Improve `check_element` helper in `test_xml_etree` (python#100934) pythonGH-101578: Normalize the current exception (pythonGH-101607) pythongh-47937: Note that Popen attributes are read-only (python#93070) ...

Try to load the fallback cmd.exe by an absolute path

d122f14

bedevere-bot added the awaiting review label Jan 24, 2023

bedevere-bot mentioned this pull request Jan 24, 2023

Undocumented risky behaviour in subprocess module #101283

Closed

Add a NEWS entry

e96457c

arhadthedev added 2 commits January 24, 2023 16:35

Fix minor grammar

b26918e

Address patchcheck CI error report

9a0fdbd

arhadthedev marked this pull request as ready for review January 24, 2023 13:46

arhadthedev requested a review from gpshead as a code owner January 24, 2023 13:46

eryksun reviewed Jan 24, 2023

View changes

Lib/subprocess.py Outdated Show resolved Hide resolved

eryksun reviewed Jan 24, 2023

View changes

Lib/subprocess.py Outdated Show resolved Hide resolved

Address the review

091e970

Co-authored-by: Eryk Sun <eryksun@gmail.com>

eryksun reviewed Jan 24, 2023

View changes

Lib/subprocess.py Outdated Show resolved Hide resolved

arhadthedev and others added 2 commits January 24, 2023 19:48

Simplify NEWS wording and reattribute the totally rewritten PR

a3cbdb6

Address the review-2

8dab7f1

Co-authored-by: Eryk Sun <eryksun@gmail.com>

eryksun reviewed Jan 24, 2023

View changes

Lib/subprocess.py Outdated Show resolved Hide resolved

Update Lib/subprocess.py

1d8d4a6

Co-authored-by: Eryk Sun <eryksun@gmail.com>

eryksun requested a review from a team January 24, 2023 17:09

eryksun added the awaiting core review label Jan 24, 2023

arhadthedev marked this pull request as draft January 24, 2023 18:48

arhadthedev marked this pull request as ready for review January 24, 2023 18:48

gpshead reviewed Jan 24, 2023

View changes

Lib/subprocess.py Outdated Show resolved Hide resolved

gpshead reviewed Jan 24, 2023

View changes

Misc/NEWS.d/next/Security/2023-01-24-16-12-00.gh-issue-101283.9tqu39.rst Outdated Show resolved Hide resolved

gpshead added type-security A security issue needs backport to 3.7 needs backport to 3.8 needs backport to 3.9 needs backport to 3.10 labels Jan 24, 2023

AlexWaygood added needs backport to 3.11 and removed needs backport to 3.11 labels Feb 8, 2023

bedevere-bot removed the needs backport to 3.11 label Feb 8, 2023

bedevere-bot removed the needs backport to 3.7 label Feb 8, 2023

zooba pushed a commit to zooba/cpython that referenced this pull request Feb 8, 2023

pythongh-101283: Improved fallback logic for subprocess with shell=Tr…

e06a55e

…ue on Windows (pythonGH-101286)

gpshead reviewed Feb 8, 2023

View changes

arhadthedev deleted the absolute-cmdexe-fallback-patch branch February 9, 2023 02:01

This was referenced Feb 9, 2023

gh-101283: Fix the versionchanged of gh-101283 (3.12 only) arhadthedev/cpython#28

Closed

gh-101283: Fix the versionchanged of gh-101283 (3.12 only) #101728

Merged

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

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

arhadthedev commented Jan 24, 2023 •

edited by bedevere-bot

arhadthedev commented Jan 24, 2023

arhadthedev commented Jan 24, 2023

gpshead Jan 24, 2023

eryksun Jan 24, 2023 •

edited

gpshead Jan 25, 2023

miss-islington commented Feb 8, 2023

miss-islington commented Feb 8, 2023

miss-islington commented Feb 8, 2023

bedevere-bot commented Feb 8, 2023

zooba commented Feb 8, 2023

zooba commented Feb 8, 2023

bedevere-bot commented Feb 8, 2023

zooba commented Feb 8, 2023

gpshead Feb 8, 2023

arhadthedev commented Feb 9, 2023

arhadthedev commented Feb 9, 2023

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

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

Conversation

arhadthedev commented Jan 24, 2023 • edited by bedevere-bot

arhadthedev commented Jan 24, 2023

arhadthedev commented Jan 24, 2023

gpshead Jan 24, 2023

Choose a reason for hiding this comment

eryksun Jan 24, 2023 • edited

Choose a reason for hiding this comment

gpshead Jan 25, 2023

Choose a reason for hiding this comment

miss-islington commented Feb 8, 2023

miss-islington commented Feb 8, 2023

miss-islington commented Feb 8, 2023

bedevere-bot commented Feb 8, 2023

zooba commented Feb 8, 2023

zooba commented Feb 8, 2023

bedevere-bot commented Feb 8, 2023

zooba commented Feb 8, 2023

gpshead Feb 8, 2023

Choose a reason for hiding this comment

arhadthedev commented Feb 9, 2023

arhadthedev commented Feb 9, 2023

arhadthedev commented Jan 24, 2023 •

edited by bedevere-bot

eryksun Jan 24, 2023 •

edited