classification
Title: subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista
Type: resource usage Stage: patch review
Components: Library (Lib), Windows Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Bernt.Røskar.Brenna, Segev Finer, eryksun, haypo, paul.moore, sbt, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2013-11-25 09:01 by haypo, last changed 2017-04-20 19:49 by python-dev.

Files
File name Uploaded Description Edit
windows-subprocess-close-fds.patch Segev Finer, 2017-01-06 12:38 Implement subprocess.Popen(close_fds=True) with stdio on Windows review
windows-subprocess-close-fds-v2.patch Segev Finer, 2017-01-06 18:19 Second version of the patch review
windows-subprocess-close-fds-v3.patch Segev Finer, 2017-01-07 08:16 Third version of the patch review
windows-subprocess-close-fds-v3-vista7-hack.patch Segev Finer, 2017-01-13 17:22 Third version with an hack around Vista/7 nastiness (Requires testing) review
windows-subprocess-close-fds-v4.patch Segev Finer, 2017-04-19 22:08 4th version of the patch review
windows-subprocess-close-fds-v5.patch Segev Finer, 2017-04-20 18:13 review
windows-subprocess-close-fds-v6.patch Segev Finer, 2017-04-20 19:09 6th version of the patch (Now on top of Git :P)
Pull Requests
URL Status Linked Edit
PR 1218 open python-dev, 2017-04-20 19:49
Messages (18)
msg204314 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-25 09:01
subprocess.Popen has a race condition on Windows with file descriptors: if two threads spawn subprocesses at the same time, unwanted file descriptors may be inherited, which lead to annoying issues like "cannot delete a file because it is open by another process". For the issue #19575 for an example of such bug.

Since Windows Vista, a list of handle which should be inherited can be specified in CreateProcess() using PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX. It avoids the need to mark the handle temporarly inheritable.

For more information, see:
http://www.python.org/dev/peps/pep-0446/#only-inherit-some-handles-on-windows
msg204637 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-28 01:35
The purpose of this issue is to avoiding having to call CreateProcess() with bInheritHandles parameter set to TRUE on Windows, and avoid calls to self._make_inheritable() in subprocess.Popen._get_handles().

Currently, bInheritHandles is set to TRUE if stdin, stdout and/or stderr parameter of Popen constructor is set (to something else than None).

Using PROC_THREAD_ATTRIBUTE_HANDLE_LIST, handles don't need to be marked as inheritable in the parent process, and CreateProcess() can be called with bInheritHandles parameter set to FALSE.
msg204638 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-28 01:40
UpdateProcThreadAttribute() documentation says that "... handles must be created as inheritable handles ..." and a comment says that "If using PROC_THREAD_ATTRIBUTE_HANDLE_LIST, pass TRUE to bInherit in CreateProcess. Otherwise, you will get an ERROR_INVALID_PARAMETER."

http://msdn.microsoft.com/en-us/library/windows/desktop/ms686880%28v=vs.85%29.aspx

Seriously? What is the purpose of PROC_THREAD_ATTRIBUTE_HANDLE_LIST if it does not avoid the race condition? It's "just" to not inherit some inheritable handles? In Python 3.4, files and sockets are created non-inheritable by default, so PROC_THREAD_ATTRIBUTE_HANDLE_LIST may not improve anything :-/
msg204675 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-28 16:35
I read again the following blog post:
http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx

I understood the purpose of PROC_THREAD_ATTRIBUTE_HANDLE_LIST.

Let say that two Python threads create a Popen object with a pipe for stdout:

* Thread A : pipe 1
* Thread B : pipe 2
* Main thread has random inheritable files and sockets

Handles of the both pipes are inheritable. Currently, thread A may inherit pipe 2 and thread B may inherit pipe 1 depending exactly when pipes are created and marked as inheritable, and when CreateProcess() is called.

Using PROC_THREAD_ATTRIBUTE_HANDLE_LIST, thread A will only inherit pipe 1, not pipe 2 nor inheritable handles of the other threads. Thread B will only inherit pipe 1, no other handle. It does not matter that CreateProcess() is called with bInheritHandles=TRUE nor that there are other inheritable handles.
msg284817 - (view) Author: Segev Finer (Segev Finer) * Date: 2017-01-06 12:38
Though Python has taken measures to mark handles as non-inheritable there is still a possible race due to having to create inheritable handles while creating processes with stdio pipes (subprocess).

Attached is a Patch that implements subprocess.Popen(close_fds=True) with stdio handles on Windows using PROC_THREAD_ATTRIBUTE_HANDLE_LIST, which plugs that race completely.

I implemented this by adding the attribute STARTUPINFO._handleList, which when passed to _winapi.CreateProcess, will be passed to CreateProcess as a PROC_THREAD_ATTRIBUTE_HANDLE_LIST. subprocess.py can than use this attribute as needed with inherit_handles=True to only inherit the stdio handles.

The STARTUPINFO._handleList attribute can also be used to implement pass_fds later on. Though the exact behavior of how to convert a file descriptor list to a handle list might be a bit sensitive, so I left that out for now.

This patch obviously doesn't support Windows XP but Python 3 doesn't support XP anymore either.
msg284834 - (view) Author: Eryk Sun (eryksun) * Date: 2017-01-06 16:45
Implementing pass_fds on Windows is a problem if Popen has to implement the undocumented use of the STARTUPINFO cbReserved2 and lpReserved2 fields to inherit CRT file descriptors. I suppose we could implement this ourselves in _winapi since it's unlikely that the data format will ever change. Just copy what the CRT's accumulate_inheritable_handles() function does, but constrained by an array of file descriptors.
msg284843 - (view) Author: Segev Finer (Segev Finer) * Date: 2017-01-06 18:19
Second version of the patch after review by eryksun.

Please pay attention to the hack in _execute_child due to having to 
temporarily override the handle_list if the user supplied
one.

As for pass_fds: as you noted, it has it's own share of complexities and issues and I think it's best to leave it to a separate patch/issue.
msg284865 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-01-06 22:42
Python already has a multiprocessing module which is able to pass handles (maybe also FD? I don't know) to child processes on Windows. I found some code in Lib/multiprocessing/reduction.py:
- duplicate()
- steal_handle()
- send_handle()

But the design doesn't really fit the subprocess module, since this design requires that the child process communicates with the parent process. On UNIX, fork()+exec() is used, so we can execute a few instructions after fork, which allows to pass an exception from the child to the parent. On Windows, CreateProcess() is used which doesn't allow directly to execute code before running the final child process.

The PEP 446 describes a solution using a wrapper process, so parent+wrapper+child, 3 processes. IMHO the best design for subprocess is really PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
msg284866 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-01-06 22:51
I dislike adding a lpAttributeList attribute: it's too close to the exact implementation of Windows may change in the future. I would prefer a more high level API.

Since the only known use case today is to pass handles, I propose to focus on this use case: add a new pass_handles parameter to Popen, similar to pass_fds.

I see that your patch is able to set close_fds to True on Windows: great job! It would be a great achievement to finally fix this last known race condition of subprocess on Windows!

So thank you for working on this!


> As for pass_fds: as you noted, it has it's own share of complexities and issues and I think it's best to leave it to a separate patch/issue.

pass_fds would be "nice to have", but I prefer to stick first to the native and well supported handles on Windows. For me, using file descriptors on Windows is more a "hack" to be able to write code working on Windows and UNIX, but since it's not natively supported on Windows, it comes with own set of issues.

IMHO it's better supported to work on handles.
msg284898 - (view) Author: Segev Finer (Segev Finer) * Date: 2017-01-07 08:16
I removed previous_handle_list in _execute_child since I noticed subprocess already clobbers the other attributes in startupinfo anyhow.

I figured there will be some discussion about how to pass the handle list, so here's my two cents:

* subprocess already exposes a bit of Windows specific flags like creationflags and STARTUPINFO.

* Windows doesn't really break it's API in backwards incompatible ways often (Heck it barely breaks it ever, which is why we have so many Ex functions and reserved parameters :P).

* The _winapi module tries to expose WinAPI functions as is. So I implemented this as an internal attribute on STARTUPINFO, in the first version, since I wasn't sure we want this exposed to users, but I still wanted to try and mimic the original WinAPI functions internally. The lpAttributeList is a change requested by eryksun that brings it even closer to WinAPI and exposes it for further extension with additional attributes.
msg284903 - (view) Author: Eryk Sun (eryksun) * Date: 2017-01-07 08:52
> Python already has a multiprocessing module which is able to pass 
> handles (maybe also FD? I don't know) to child processes on 
> Windows.

Popen doesn't implement the undocumented CRT protocol that's used to smuggle the file-descriptor mapping in the STARTUPINFO cbReserved2 and lpReserved2 fields. This is a feature of the CRT's spawn and exec functions. For example:

    fdr, fdw = os.pipe()
    os.set_inheritable(fdw, True)
    os.spawnl(os.P_WAIT, os.environ['ComSpec'], 'cmd /c "echo spam >&%d"' % fdw)

    >>> os.read(fdr, 10)
    b'spam \r\n'

We don't have to worry about implementing fd inheritance so long as os.spawn* uses the CRT. Someone that needs this functionality can simply be instructed to use os.spawn.

> I dislike adding a lpAttributeList attribute: it's too close to 
> the exact implementation of Windows may change in the future.

If you're going to worry about lpAttributeList, why stop there? 
Aren't dwFlags, wShowWindow, hStdInput, hStdOutput, and hStdError also too close to the exact implementation? My thoughts when suggesting this were actually to make this as close to the underlying API as possible, and extensible to support other attributes if there's a demand for it. 

Passing a list of handles is atypical usage, and since Python and subprocess use file descriptors instead of Windows handles, I prefer isolating this in a Windows structure such as STARTUPINFO, rather than adding even more confusion to Popen's constructor.

> Since the only known use case today is to pass handles

In the review of the first patch, I listed 3 additional attributes that might be useful to add in 3.7: IDEAL_PROCESSOR, GROUP_AFFINITY, and PREFERRED_NODE (simplified by the fact that 3.7 no longer supports Vista). Currently the way to set the latter two is to use the built-in `start` command of the cmd shell.

> I propose to focus on this use case: add a new pass_handles parameter
> to Popen, similar to pass_fds.

This is a messy situation. Python 3's file I/O is built on the CRT's POSIX layer. If it had been implemented directly on the Windows API using handles, then pass_fds would obviously use handles. That's the current situation with socket module because Winsock makes no attempt to hide AFD handles behind POSIX file descriptors. 

Popen's constructor accepts file descriptors -- not Windows handles -- for its stdin, stdout, and stderr arguments, and the parameter to control inheritance is named "close_fds". It seems out of place to add a "pass_handles" parameter.
msg285427 - (view) Author: Segev Finer (Segev Finer) * Date: 2017-01-13 17:22
I have read some of https://github.com/rprichard/win32-console-docs and it documents quite a bunch of nastiness with PROC_THREAD_ATTRIBUTE_HANDLE_LIST in Windows Vista/7. Windows is so much fun sometimes :P

Essentially console handles in Windows before Windows 8 are user mode handles and not real kernel handles. Those user mode handles are inherited by a different mechanism than kernel handles and regardless of PROC_THREAD_ATTRIBUTE_HANDLE_LIST, and if passed in PROC_THREAD_ATTRIBUTE_HANDLE_LIST will cause it to fail in weird ways. Those user mode console handles have the lower two bits set. The lower two bits in Windows are reserved for tagging such special handles.

Also in all versions you can't pass in an empty handle list, but a list with just a NULL handle works fine.

See: https://github.com/rprichard/win32-console-docs/blob/master/src/HandleTests/CreateProcess_InheritList.cc

I attached a version of the patch with a hack around those issues based on what I read, but I can't test that it actually fixes the issues since I don't have a Windows Vista or 7 system around.
msg291421 - (view) Author: Segev Finer (Segev Finer) * Date: 2017-04-10 08:26
It's been a while since this got any attention...
msg291900 - (view) Author: Eryk Sun (eryksun) * Date: 2017-04-19 20:27
In case you didn't get notified by Rietveld, I made a couple suggestions on your latest patch. Also, if you wouldn't mind, please update the patch to apply cleanly to 3.7 -- especially since STARTUPINFO now has an __init__ method.
msg291915 - (view) Author: Segev Finer (Segev Finer) * Date: 2017-04-19 22:08
Added the 4th version after review by eryksun (In rietveld).
msg291989 - (view) Author: Segev Finer (Segev Finer) * Date: 2017-04-20 18:13
Added the 5th version after another review by eryksun (In rietveld).
msg291995 - (view) Author: Segev Finer (Segev Finer) * Date: 2017-04-20 19:09
Oh LOL!!! I missed the fact that Python finally moved to GitHub!
Rebased the patch on top of the Git master XD (And removed accidentally committed code... sorry...)

I still submitted as a patch since I don't know if the infrastructure handles moving a patch to a PR well :P
msg291996 - (view) Author: Segev Finer (Segev Finer) * Date: 2017-04-20 19:48
OK Rietveld definitely punted on the git patch (I guess it's only for the old Mercurial repo, I don't think it actually even support Git...)

I will try re-submitting the patch as a PR so that it can be reviewed easily.
History
Date User Action Args
2017-04-20 19:49:07python-devsetpull_requests: + pull_request1341
2017-04-20 19:48:32Segev Finersetmessages: + msg291996
2017-04-20 19:09:00Segev Finersetfiles: + windows-subprocess-close-fds-v6.patch

messages: + msg291995
2017-04-20 18:13:06Segev Finersetfiles: + windows-subprocess-close-fds-v5.patch

messages: + msg291989
2017-04-19 22:08:24Segev Finersetfiles: + windows-subprocess-close-fds-v4.patch

messages: + msg291915
2017-04-19 20:27:26eryksunsetmessages: + msg291900
2017-04-10 08:26:40Segev Finersetmessages: + msg291421
2017-01-13 17:22:31Segev Finersetfiles: + windows-subprocess-close-fds-v3-vista7-hack.patch

messages: + msg285427
2017-01-07 08:52:39eryksunsetmessages: + msg284903
2017-01-07 08:16:35Segev Finersetfiles: + windows-subprocess-close-fds-v3.patch

messages: + msg284898
2017-01-06 22:51:51hayposetmessages: + msg284866
2017-01-06 22:42:51hayposetmessages: + msg284865
2017-01-06 18:19:47Segev Finersetfiles: + windows-subprocess-close-fds-v2.patch

messages: + msg284843
2017-01-06 16:45:17eryksunsetversions: + Python 3.7, - Python 3.5
nosy: + eryksun

messages: + msg284834

components: + Library (Lib)
stage: patch review
2017-01-06 12:38:36Segev Finersetfiles: + windows-subprocess-close-fds.patch

nosy: + Segev Finer
messages: + msg284817

keywords: + patch
2016-04-12 23:02:40hayposetnosy: + paul.moore, tim.golden, zach.ware, steve.dower
type: enhancement -> resource usage
components: + Windows
2013-11-28 16:35:02hayposetmessages: + msg204675
2013-11-28 01:40:39hayposetmessages: + msg204638
2013-11-28 01:35:30hayposetmessages: + msg204637
2013-11-25 09:01:55haypocreate