subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

[vstinner]

BPO	19764
Nosy	@gpshead, @pfmoore, @vstinner, @tjguk, @codeape2, @zware, @eryksun, @zooba, @xflr6, @segevfiner
PRs	bpo-19764: Implemented support for subprocess.Popen(close_fds=True) on Windows #1218
Files	windows-subprocess-close-fds.patch: Implement subprocess.Popen(close_fds=True) with stdio on Windows windows-subprocess-close-fds-v2.patch: Second version of the patch windows-subprocess-close-fds-v3.patch: Third version of the patch windows-subprocess-close-fds-v3-vista7-hack.patch: Third version with an hack around Vista/7 nastiness (Requires testing) windows-subprocess-close-fds-v4.patch: 4th version of the patch windows-subprocess-close-fds-v5.patch windows-subprocess-close-fds-v6.patch: 6th version of the patch (Now on top of Git :P)

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-12-18.09:35:37.441>
created_at = <Date 2013-11-25.09:01:55.948>
labels = ['3.7', 'library', 'OS-windows', 'performance']
title = 'subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista'
updated_at = <Date 2018-07-04.14:56:46.872>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2018-07-04.14:56:46.872>
actor = 'xflr6'
assignee = 'none'
closed = True
closed_date = <Date 2017-12-18.09:35:37.441>
closer = 'vstinner'
components = ['Library (Lib)', 'Windows']
creation = <Date 2013-11-25.09:01:55.948>
creator = 'vstinner'
dependencies = []
files = ['46175', '46178', '46185', '46282', '46814', '46819', '46820']
hgrepos = []
issue_num = 19764
keywords = ['patch']
message_count = 28.0
messages = ['204314', '204637', '204638', '204675', '284817', '284834', '284843', '284865', '284866', '284898', '284903', '285427', '291421', '291900', '291915', '291989', '291995', '291996', '294164', '294791', '294810', '308470', '308528', '308530', '308531', '320784', '320806', '321048']
nosy_count = 11.0
nosy_names = ['gregory.p.smith', 'paul.moore', 'vstinner', 'tim.golden', 'Bernt.R\xc3\xb8skar.Brenna', 'sbt', 'zach.ware', 'eryksun', 'steve.dower', 'xflr6', 'Segev Finer']
pr_nums = ['1218']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue19764'
versions = ['Python 3.7']

[vstinner]

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 bpo-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

[vstinner]

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.

[vstinner]

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 :-/

[vstinner]

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.

[segevfiner]

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.

[eryksun]

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.

[segevfiner]

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.

[vstinner]

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.

[vstinner]

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.