bpo-19764: Implemented support for subprocess.Popen(close_fds=True) on Windows #1218

segevfiner · 2017-04-20T19:49:06Z

Even though Python marks any handles it opens as non-inheritable there
is still a race when using subprocess.Popen since creating a process
with redirected stdio requires temporarily creating inheritable handles.
By implementing support for subprocess.Popen(close_fds=True) we fix
this race.

In order to implement this we use PROC_THREAD_ATTRIBUTE_HANDLE_LIST
which is available since Windows Vista. Which allows to pass an explicit
list of handles to inherit when creating a process.

This commit also adds STARTUPINFO.lpAttributeList["handle_list"]
which can be used to control PROC_THREAD_ATTRIBUTE_HANDLE_LIST directly.

https://bugs.python.org/issue19764

the-knights-who-say-ni · 2017-04-20T19:49:08Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

mention-bot · 2017-04-20T19:49:11Z

@segevfiner, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gpshead, @zware and @serhiy-storchaka to be potential reviewers.

segevfiner · 2017-04-20T19:56:01Z

@eryksun

eryksun · 2017-04-20T20:19:32Z

Lib/subprocess.py

+                handle_list = attribute_list.setdefault("handle_list", [])
+
+                if use_std_handles:
+                    handle_list += [p2cread, c2pwrite, errwrite]


It's probably better to add these handles as [int(p2cread), int(c2pwrite), int(errwrite)]. The subprocess.Handle instances aren't needed here.

Also, maybe it needs another issue, but I noticed that it's not setting self._closed_child_pipe_fds = True after closing the handles in the finally block in _execute_child. Consequently the except block in Popen.__init__ tries to call os.close on a Windows handle when CreateProcess fails. In a debug build this raises a failed assertion dialog.

Actually, I vaguely remember seeing this debug assertion too, but I didn't look into it then. I will submit a separate issue and PR for this.

It actually seems to be worse! 😝 It's calling os.close on an HANDLE (os.close is for file descriptors, emulated in user mode by msvcrt).

See: Issue #30121

Yes, I take for granted that we all know it's a bad thing to call os.close on a Windows handle. I noticed it because self._closed_child_pipe_fds = True isn't being set, but it needs to be fixed in both places because in principle instantiating Popen could fail before the try/finally in _execute_child.

eryksun · 2017-04-21T05:26:43Z

Lib/subprocess.py

+                handle_list[:] = self._filter_handle_list(handle_list)
+
+                if handle_list:
+                    if have_handle_list and not close_fds:


This should just check not close_fds because have_handle_list must be true if that's the case, on account of the outer check on line 989.

You can get here with have_handle_list = False by not passing an handle_list explicitly and requesting stdio redirection.

import subprocess subprocess.Popen(['notepad.exe'], stdout=subprocess.PIPE)

The condition on line 989 is for either have_handle_list or having requested stdio redirection with close_fds.

have_handle_list really means that the user has explicitly passed in a non empty handle_list. I placed the check for the warning there instead of someplace else, that might have been clearer, so that we will only give the warning if we actually override close_fds. Maybe a different name for have_handle_list will make this clearer? If you can come up with one...

We want the warning when using handle_list with close_fds overridden as false, which can only occur when have_handle_list is true. If we get to the check on line 1000 and close_fds is false, then we already know that have_handle_list is true. We don't need to check it.

Whoops the if there was wrong in a different way... not close_fds should have been close_fds and have_handle_list was pointing to have reference to handle_list which was later modified also changing have_handle_list.

The following should not generate a warning:

>>> import subprocess >>> subprocess.Popen(['notepad.exe'], stdout=subprocess.PIPE) <subprocess.Popen object at 0x03A0FFC0> >>> # No warning

The following should:

>>> import os, subprocess, msvcrt >>> f=open('bla.txt', 'w') >>> handle = msvcrt.get_osfhandle(f.fileno()) >>> startupinfo = subprocess.STARTUPINFO() >>> startupinfo.lpAttributeList["handle_list"] = [handle] >>> os.set_inheritable(f.fileno(), True) >>> subprocess.Popen(['notepad.exe'], startupinfo=startupinfo) C:\Users\Segev\prj\python\cpython\lib\subprocess.py:1002: RuntimeWarning: startupinfo.lpAttributeList['handle_list'] overriding close_fds "overriding close_fds", RuntimeWarning) <subprocess.Popen object at 0x03B0E9A0> >>>

If I remove have_handle_list from the if than you get a warning for the first example:

>>> import subprocess >>> subprocess.Popen(['notepad.exe'], stdout=subprocess.PIPE) C:\Users\Segev\prj\python\cpython\lib\subprocess.py:1002: RuntimeWarning: startupinfo.lpAttributeList['handle_list'] overriding close_fds "overriding close_fds", RuntimeWarning) <subprocess.Popen object at 0x02C0FFC0> >>>

At the level of the subprocess API, close_fds means the child will not inherit all of the inheritable handles of the current process, except for the standard handles, and if close_fds is false, it means all inheritable handles will be inherited. Using an explicit handle_list inherits only a given subset of handles and, like pass_fds, it's acting like an extension of the standard set of 3 handles or file descriptors. As such, it should be used with close_fds as true. For pass_fds a warning is raised when close_fds is false because it's a mixed message and the code is opting to override close_fds. For consistency, using handle_list with close_fds as false should raise a similar warning. I know it's different in the Windows API, which instead modifies what bInheritHandles means when given a handle list. But I think the goal in subprocess.Popen is try to make the semantics as universal as possible. pass_fds and handle_list are obviously different, but they share the semantics of the common close_fds parameter.

As far as I can see -- both from reading and testing -- removing have_handle_list from the if statement should never cause a warning to be raised when only redirecting the standard handles. If there's no explicitly passed handle_list, this code block executes only when close_fds is true. So if you're checking not close_fds to gate the warning, there's no way it will be raised. If close_fds is false when redirecting the standard handles, normal inheritance of the entire process handle table is used -- unless of course a handle_list is already present.

Haaaa. I misunderstood that you wanted the warning for the close_fds=True which, as you stated, is kinda confusing in regards to portable behavior.

…n Windows Even though Python marks any handles it opens as non-inheritable there is still a race when using `subprocess.Popen` since creating a process with redirected stdio requires temporarily creating inheritable handles. By implementing support for `subprocess.Popen(close_fds=True)` we fix this race. In order to implement this we use PROC_THREAD_ATTRIBUTE_HANDLE_LIST which is available since Windows Vista. Which allows to pass an explicit list of handles to inherit when creating a process. This commit also adds `STARTUPINFO.lpAttributeList["handle_list"]` which can be used to control PROC_THREAD_ATTRIBUTE_HANDLE_LIST directly.

Also fixed the documentation for handle_list.

…s=True

segevfiner · 2017-04-21T16:41:33Z

BTW. I'm not sure if you commonly do this for CPython. but I can squash the fixup commits once you are done reviewing, if you wish.

zware · 2017-04-21T16:51:34Z

Please do not squash, rebase, or otherwise overwrite the history of your PR. It will be squashed by the core developer who merges it.

segevfiner · 2017-04-21T16:53:07Z

@zware This is why I asked rather than doing it 👍

vstinner · 2017-05-29T10:35:24Z

@zooba, @eryksun: Would you mind to review this change. IMHO it's a major enhancement for Windows of the subprocess module. It would solve old an important use cases which currently requires an ugly close_fds=False workaround. See http://bugs.python.org/issue19764 for the rationale.

zooba

Couple of minor comments, but looks good to me.

zooba · 2017-05-31T16:16:44Z

Modules/_winapi.c

+        goto cleanup;
+
+    for (i = 0; i < PySequence_Fast_GET_SIZE(value_fast); i++) {
+        ret[i] = PYNUM_TO_HANDLE(PySequence_Fast_GET_ITEM(value_fast, i));


PYNUM_TO_HANDLE (actually PyLong_AsUnsignedLong[Long]()) could raise OverflowError or TypeError, which should really cause us to break out of the loop immediately. Otherwise we may end up masking the error or hitting a codepath that causes an immediate failure with an error set (though this seems unlikely here).

zooba · 2017-05-31T16:19:37Z

Modules/_winapi.c

+    PyObject *value;
+    Py_ssize_t handle_list_size;
+    DWORD attribute_count = 0;
+    SIZE_T attribute_list_size;


Should be initialized (yes, the first use is as an out parameter, but it's actually annotated as in/out and so should be set to something before calling)

zooba · 2017-05-31T16:20:51Z

Modules/_winapi.c

+        attribute_list->attribute_list = NULL;
+
+        ret = -1;
+        PyErr_SetFromWindowsErr(GetLastError());


Need to call GetLastError before PyMem_Free, which might overwrite the error code.

codecov · 2017-05-31T16:40:56Z

Codecov Report

Merging #1218 into master will decrease coverage by 0.76%.
The diff coverage is 8.77%.

@@            Coverage Diff             @@
##           master    #1218      +/-   ##
==========================================
- Coverage   83.44%   82.68%   -0.77%     
==========================================
  Files        1367     1432      +65     
  Lines      346128   353486    +7358     
==========================================
+ Hits       288823   292272    +3449     
- Misses      57305    61214    +3909

Impacted Files	Coverage Δ
Lib/subprocess.py	`55.88% <10%> (-0.71%)`	⬇️
Lib/test/test_subprocess.py	`88.23% <8.1%> (-1.85%)`	⬇️
Lib/__future__.py	`6.45% <0%> (-90.33%)`	⬇️
Lib/idlelib/idle_test/test_help_about.py	`54.76% <0%> (-42.3%)`	⬇️
Lib/test/libregrtest/runtest.py	`63.01% <0%> (-4.11%)`	⬇️
Lib/sre_compile.py	`84.06% <0%> (-3.69%)`	⬇️
Lib/re.py	`54.68% <0%> (-1.61%)`	⬇️
Lib/idlelib/idle_test/test_textview.py	`2.94% <0%> (-1.61%)`	⬇️
Lib/test/test_select.py	`87.69% <0%> (-1.54%)`	⬇️
Lib/test/libregrtest/cmdline.py	`93.18% <0%> (-1.31%)`	⬇️
... and 198 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28205b2...c84449c. Read the comment docs.

segevfiner · 2017-06-04T17:34:59Z

@zooba I implemented the fixes. (Just in case GitHub didn't notify you for some reason 😖)

zooba · 2017-06-05T19:53:23Z

I'd like @eryksun's approval as well, since he did a couple of reviews already.

We can ignore codecov here - the amount of new Windows-only code is the likely cause of the decreased coverage (codecov runs on Linux).

vstinner · 2017-06-06T15:15:49Z

Doc/library/subprocess.rst

+
+      **handle_list**
+         When *close_fds* is :const:`True`, supplies a list of
+         handles that will be inherited.


I suggest the doc:

"Sequence of handles that will be inherited. close_fds must be true if non-empty."

(And make sure that any sequence type is accepted.)

vstinner · 2017-06-06T15:16:52Z

Lib/subprocess.py

@@ -134,6 +134,7 @@ def __init__(self, *, dwFlags=0, hStdInput=None, hStdOutput=None,
            self.hStdOutput = hStdOutput


It may be convenient to be able to pass lpAttributeList in the constructor.

For convenience, do you think the handles in handle_list should be automatically duplicated as inheritable via _make_inheritable? The problem with that is managing the lifetime of the Handle instances. It would have to make a private copy of STARTUPINFO, which maybe it should be doing anyway.

I think the caller would expect the handles he passes to be inherited and not a duplicate of them. The usage is to be able to selectively inherit some specific handles after all. A script using this is very likely to pass the handles he passed to handle_list in the command line or some other IPC mechanism for the usage of the sub process, otherwise what is the point of inheriting those handles besides leaking them?

Imagine a process opening some anonymous shared memory and passing that handle on the command line for a new process, making sure to properly inherit it via handle_list. The new process will have access to the shared memory and no other additional handles which it won't even know about, unless any others are passed.

You're absolutely right. I was only thinking narrowly about a user passing in a handle list without considering the context.

Can you update the docs for handle_list to note that the handles in the list have to be made inheritable via os.set_handle_inheritable, else OSError will be raised for the error code ERROR_INVALID_PARAMETER (87). Otherwise users may expect it to work like the pass_fds parameter. Including the error code in the docs may help with searching, since it's such a generic error.

Do you think it needs a warning that concurrent calls to CreateProcess that inherit all handles may inadvertently inherit the handles in this list? The race condition can only be avoided with careful design, such as by always using Popen with a handle list and avoiding concurrent calls to os.system and os.spawn* or by using a lock to synchronize process creation.

I tried to add some more documentation about this but I'm not really sure it's the right place for it. Feel free to suggest better wording or a better place for it.

vstinner · 2017-06-06T15:17:25Z

Lib/subprocess.py

@@ -608,25 +609,15 @@ def __init__(self, args, bufsize=-1, executable=None,
        if not isinstance(bufsize, int):
            raise TypeError("bufsize must be an integer")

+        if close_fds is _PLATFORM_DEFAULT_CLOSE_FDS:
+            close_fds = True


I suggest to now remove _PLATFORM_DEFAULT_CLOSE_FDS and always set default to True.

vstinner · 2017-06-06T15:18:59Z

Lib/subprocess.py

+        def _filter_handle_list(self, handle_list):
+            """Filter out console handles that can't be used
+            in lpAttributeList["handle_list"] and make sure the list
+            isn't empty"""


Please document also that it removes duplicate handles. (Move the comment into the docstring.)

vstinner · 2017-06-06T15:25:18Z

I suggest to document the change in Doc/whatsnew/3.7.rst. IMHO it's a major change.

Is there a risk of backward incompatibility issue? If yes, mention also the change in the "Porting to Python 3.7" section.

segevfiner · 2017-06-06T18:30:45Z

Should I rebase or merge to fix conflicts?

serhiy-storchaka · 2017-06-06T18:34:29Z

Merge.

…s-close-fds

vstinner · 2017-06-07T08:41:32Z

The pass_fds file descriptors are made inheritable in the forked child prior to exec, so there's no chance of a race condition in the current process. For Windows, it could automate making the handles in the list inheritable in the current process, but that would need a warning in the docs, no?

Oh, I see. If there is a risk of race condition: don't do anything, raise an exception and document the requirement. asyncio does something similar to sockets expected to be non-blocking.

eryksun · 2017-06-07T20:45:10Z

Doc/library/subprocess.rst

+         Sequence of handles that will be inherited. *close_fds* must be true if
+         non-empty.
+
+         The handles must be made inheritable by


You could say "The handles must be temporarily made inheritable by os.set_handle_inheritable when passed to the Popen constructor, else OSError will be raised...". Then the second sentence can be removed.

The second paragraph I think should be a warning. How about this?

.. warning:: In a multithreaded process, use caution to avoid leaking handles when combining this feature with concurrent calls to other process creation functions that inherit all handles such as :func:`os.system`. This also applies to standard handle redirection, which temporarily creates inheritable handles.

…s-close-fds

segevfiner · 2017-06-10T07:45:00Z

Merged from master due to argument clinic changes.

…s-close-fds

segevfiner · 2017-06-24T18:40:20Z

@eryksun Is there anything else that you think should be improved/fixed? It would be nice to know if everything seems fine. 😉

…s-close-fds

gpshead

While I'm not in a position to comment on Windows specifics, I think what you have done here is a good thing and will make Windows users happy. +1 for accepting this change. I must defer to someone with actual Windows knowledge to press the merge button. Your documentation and common subprocess.py code dealing with the formerly different default close_fds behavior updates look reasonable to me.

…s-close-fds

segevfiner · 2017-11-17T12:09:04Z

Applied some anti bit rot grease...

…s-close-fds

segevfiner · 2017-12-16T10:10:22Z

Merged from master... Again... Hopefully this won't end up missing 3.7 entirely... 😔

I guess CPython is completely swamped what with the PR list getting bigger rather than smaller over time...

vstinner · 2017-12-18T09:31:45Z

Merged from master... Again... Hopefully this won't end up missing 3.7 entirely... 😔

Oops sorry, I wanted this feature but I didn't follow closely the PR.

I don't know well the Windows API, so I didn't want to take the responsability of reviewing (approving) such PR. But I see that @zooba and @gpshead approved it, so I'm now confortable to merge it :-) Moreover, AppVeyor validated the PR, so let me merge it.

I prefer to merge the PR right now to not miss the Python 3.7 feature freeze, and maybe fix issues later if needed, before 3.7 final.

Thank you @segevfiner for this major subprocess enhancement. I really love to see close_fds default changing to True on Windows. It will help to fix many corner cases which are very tricky to debug.

Sorry for the slow review, but the subprocess is a critical module of Python, and we lack of Windows developers to review changes specific to Windows.

the-knights-who-say-ni added the CLA not signed label Apr 20, 2017

segevfiner force-pushed the windows-subprocess-close-fds branch from 2573232 to cd2a322 Compare April 20, 2017 19:52

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Apr 20, 2017

eryksun reviewed Apr 20, 2017

View reviewed changes

eryksun reviewed Apr 21, 2017

View reviewed changes

segevfiner added 3 commits April 21, 2017 13:03

bpo-19764: Convert subprocess.Handle to int in handle_list

ad07b10

bpo-19764: Fixed the handle_list warning with close_fds=True

9ad94f3

Also fixed the documentation for handle_list.

segevfiner force-pushed the windows-subprocess-close-fds branch from af4e1d5 to 9ad94f3 Compare April 21, 2017 10:03

segevfiner added 2 commits April 21, 2017 13:35

bpo-19764: Added a test for the overriding close_fds warning

24f3f39

bpo-19764: The warning should be emitted for handle_list and close_fd…

1eab242

…s=True

zooba approved these changes May 31, 2017

View reviewed changes

Minor fixes in _winapi.c

5b3cccc

vstinner reviewed Jun 6, 2017

View reviewed changes

bpo-19764: Addressed review comments by haypo

c84449c

Merge remote-tracking branch 'upstream/master' into windows-subproces…

f849f92

…s-close-fds

bpo-19764: Add some extra documentation about handle_list

167590b

eryksun reviewed Jun 7, 2017

View reviewed changes

segevfiner added 2 commits June 8, 2017 00:25

bpo-19764: Documentation update

59af91f

bpo-19764: Fixed bad reStructuredText markup

d4a9cdb

segevfiner force-pushed the windows-subprocess-close-fds branch from 4ce6fd0 to d4a9cdb Compare June 8, 2017 16:44

Merge remote-tracking branch 'upstream/master' into windows-subproces…

15424fa

…s-close-fds

Merge remote-tracking branch 'upstream/master' into windows-subproces…

dc1e551

…s-close-fds

segevfiner added 5 commits July 2, 2017 23:46

Merge branch 'master' into windows-subprocess-close-fds

d99af74

Merge remote-tracking branch 'upstream/master' into windows-subproces…

c63e045

…s-close-fds

Merge branch 'master' into windows-subprocess-close-fds

f50ca37

Merge remote-tracking branch 'upstream/master' into windows-subproces…

38b4526

…s-close-fds

bpo-19764: Fix whitespace

8e5d21f

segevfiner requested a review from gpshead as a code owner August 18, 2017 14:56

bpo-19764: Add NEWS.d entry

208274c

gpshead approved these changes Aug 29, 2017

View reviewed changes

bedevere-bot added the awaiting merge label Aug 29, 2017

segevfiner added 2 commits September 19, 2017 13:10

Merge branch 'master' into windows-subprocess-close-fds

869b54c

Merge remote-tracking branch 'upstream/master' into windows-subproces…

66b74f6

…s-close-fds

segevfiner added 2 commits December 16, 2017 12:05

Merge remote-tracking branch 'upstream/master' into windows-subproces…

ba19bd8

…s-close-fds

Add Segev Finer to Misc/ACKS

3b0426e

vstinner merged commit b2a6083 into python:master Dec 18, 2017

bedevere-bot removed the awaiting merge label Dec 18, 2017

segevfiner deleted the windows-subprocess-close-fds branch January 9, 2018 21:34

alecmev mentioned this pull request Aug 13, 2022

Add --node-ipc support sublimelsp/LSP#2015

Draft

		@@ -134,6 +134,7 @@ def __init__(self, *, dwFlags=0, hStdInput=None, hStdOutput=None,
		self.hStdOutput = hStdOutput

bpo-19764: Implemented support for subprocess.Popen(close_fds=True) on Windows #1218

bpo-19764: Implemented support for subprocess.Popen(close_fds=True) on Windows #1218

Conversation

segevfiner commented Apr 20, 2017 • edited by bedevere-bot Loading

the-knights-who-say-ni commented Apr 20, 2017

mention-bot commented Apr 20, 2017

segevfiner commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

segevfiner commented Apr 21, 2017

zware commented Apr 21, 2017

segevfiner commented Apr 21, 2017

vstinner commented May 29, 2017

zooba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 31, 2017 • edited Loading

Codecov Report

segevfiner commented Jun 4, 2017

zooba commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Jun 6, 2017

segevfiner commented Jun 6, 2017

serhiy-storchaka commented Jun 6, 2017

vstinner commented Jun 7, 2017

Choose a reason for hiding this comment

segevfiner commented Jun 10, 2017

segevfiner commented Jun 24, 2017

gpshead left a comment

Choose a reason for hiding this comment

segevfiner commented Nov 17, 2017

segevfiner commented Dec 16, 2017

vstinner commented Dec 18, 2017

segevfiner commented Apr 20, 2017 •

edited by bedevere-bot

Loading

codecov bot commented May 31, 2017 •

edited

Loading