Skip to content

bpo-34044: subprocess.Popen copies startupinfo #8090

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
vstinner merged 2 commits into from
Jul 5, 2018
Merged

bpo-34044: subprocess.Popen copies startupinfo #8090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d573808
bpo-34044: subprocess.Popen copies startupinfo
vstinner Jul 4, 2018
d8490f7
STARTUPINFO.copy(): use dict.copy()
vstinner Jul 5, 2018
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
17 changes: 17 additions & 0 deletions Lib/subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,19 @@ def __init__(self, *, dwFlags=0, hStdInput=None, hStdOutput=None,
self.hStdError = hStdError
self.wShowWindow = wShowWindow
self.lpAttributeList = lpAttributeList or {"handle_list": []}

def copy(self):

@segevfiner segevfiner Jul 4, 2018

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.

This does a deep copy. Maybe it's better to implement this using copy.deepcopy? (That's how I thought about doing it when I originally discovered that subprocess modifies the STARTUPINFO)

That way there will be no need to manually deep copy specific attributes by name below. You can just copy.deepcopy(self.lpAttributeList, memo). This should boil this entire method into one statement.

@vstinner vstinner Jul 5, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IMHO copy.deepcopy() is a bad practice. It seems like lpAttributeList can only contain one key, "handle_list", and the value of this key is always converted to a list in Popen (simplified code):

handle_list = list(attribute_list.get("handle_list", []))

@vstinner vstinner Jul 5, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

copy.deepcopy() is only called 3 times in the whole stdlib... in dataclasses and mailbox.

@segevfiner segevfiner Jul 5, 2018

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.

I also came up with this alternative:

attribute_list = self.lpAttributeList.copy()
if 'handle_list' in attribute_list:
    attribute_list['handle_list'] = list(attribute_list['handle_list'])

It's shorter, and dict.copy might be faster than iterating and assigning to a new dict (I didn't check...) Use whichever one you like best. 😉

@vstinner vstinner Jul 5, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

attr_list = self.lpAttributeList.copy()
if 'handle_list' in attr_list:
attr_list['handle_list'] = list(attr_list['handle_list'])

return STARTUPINFO(dwFlags=self.dwFlags,
hStdInput=self.hStdInput,
hStdOutput=self.hStdOutput,
hStdError=self.hStdError,
wShowWindow=self.wShowWindow,
lpAttributeList=attr_list)

else:
import _posixsubprocess
import select
Expand Down Expand Up @@ -1102,6 +1115,10 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
# Process startup details
if startupinfo is None:
startupinfo = STARTUPINFO()
else:
# bpo-34044: Copy STARTUPINFO since it is modified above,
# so the caller can reuse it multiple times.
startupinfo = startupinfo.copy()

use_std_handles = -1 not in (p2cread, c2pwrite, errwrite)
if use_std_handles:
Expand Down
27 changes: 27 additions & 0 deletions Lib/test/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2822,6 +2822,33 @@ def test_startupinfo_keywords(self):
subprocess.call([sys.executable, "-c", "import sys; sys.exit(0)"],
startupinfo=startupinfo)

def test_startupinfo_copy(self):
# bpo-34044: Popen must not modify input STARTUPINFO structure
startupinfo = subprocess.STARTUPINFO()
startupinfo.dwFlags = subprocess.STARTF_USESHOWWINDOW
startupinfo.wShowWindow = subprocess.SW_HIDE

# Call Popen() twice with the same startupinfo object to make sure
# that it's not modified
for _ in range(2):
cmd = [sys.executable, "-c", "pass"]
with open(os.devnull, 'w') as null:
proc = subprocess.Popen(cmd,
stdout=null,
stderr=subprocess.STDOUT,
startupinfo=startupinfo)
with proc:
proc.communicate()
self.assertEqual(proc.returncode, 0)

self.assertEqual(startupinfo.dwFlags,
subprocess.STARTF_USESHOWWINDOW)
self.assertIsNone(startupinfo.hStdInput)
self.assertIsNone(startupinfo.hStdOutput)
self.assertIsNone(startupinfo.hStdError)
self.assertEqual(startupinfo.wShowWindow, subprocess.SW_HIDE)
self.assertEqual(startupinfo.lpAttributeList, {"handle_list": []})

def test_creationflags(self):
# creationflags argument
CREATE_NEW_CONSOLE = 16
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2018-07-04-17-14-26.bpo-34044.KWAu4y.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
``subprocess.Popen`` now copies the *startupinfo* argument to leave it
unchanged: it will modify the copy, so that the same ``STARTUPINFO`` object can
be used multiple times.