[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[Jack O'Connor]

Changes by Jack O'Connor :


--
nosy: +oconnor663

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[STINNER Victor]

STINNER Victor added the comment:

> The one potential problem that I see is that it looks like specifying 
> PROC_THREAD_ATTRIBUTE_HANDLE_LIST means that no other handles are inherited, 
> even if they are inheritable.

We can maybe add a parameter to configure the behaviour: keep current behaviour 
by default in Python 3.6 and change the default in Python 3.7 for example.

> I could add Windows support for pass_fds, (...)

Yes, we need a new "pass_handles" parameter to pass a list of handles which 
will be explicitly inherited. This parameter is useful even if you inherit all 
inheritable handles, because handles can be non-inhertable too.

It's the same with pass_fds: if you pass a file descriptor in pass_fds, it will 
be marked as inheritable to be passed to the child.

I guess that "pass_handles" will be implemented with 
PROC_THREAD_ATTRIBUTE_HANDLE_LIST, or by making handles temporary inheritable 
if PROC_THREAD_ATTRIBUTE_HANDLE_LIST is not available.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[Adam Meily]

Adam Meily added the comment:

Attached is a test Python script that you can run to see the race condition in 
action. There are two Python scripts: pipe.py and reader.py.

 - pipe.py: make two subprocess.Popen() calls from two different threads.

 - reader.py: (its content is in the bottom comments of pipe.py, so move it to 
a separate file): reads lines from stdin and exits

Basically, pipe.py creates a pipe between echo Hello World! and python 
reader.py.

You'll see that pipe.py will hang until ctrl+c is entered, even though the 
first subprocess in the pipe, reader.py, exits. Uncommenting the time.sleep() 
call will cause the first subprocess.Popen() time to properly close the 
inheritable handles before the second subprocess.Popen() call is made in the 
other thread, thus the inheritable handles are leaked.

--
Added file: http://bugs.python.org/file40220/pipe.py

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24909
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[Adam Meily]

New submission from Adam Meily:

** This issue and attached patch only affect Windows **

Currently, the Popen constructor will duplicate any stdout, stdin, and/or 
stderr handle passed in and make them inheritable, by calling DuplicateHandle. 
If two threads call Popen at the same time, the newly created inheritable 
handles will leak into the subprocess that's running being created in the 
opposite thread. This has consequences when two or more subprocesses are piped 
together and executed at the time time.


Example
===

A pipe is created using the os.pipe() function. Then, two threads are started, 
T1 and T2. T1 calls Popen, setting stdout to the write-end of the pipe. T2 
calls Popen, setting stdin to the read-end of the pipe. Stock CPython would 
make T1.stdout and T2.stdin inheritable. T1's Popen will create a subprocess, 
which correctly inherits T1.stdout, but it will also inherit the T2.stdin 
handle. Likewise, T2's Popen will create a subprocess, which correctly inherits 
T2.stdin, but it will also inherit the T1.stdout handle. Thus, in this 
situation where both ends of the pipe were accidentally inherited, the pipe 
will not properly close and T2's subprocess will have to be forcefully killed 
(assuming it is reading from stdin).

The patch simply enforces that the lifetime of an inheritable handle is limited 
to a single call to Popen.


Why this should be in CPython
=

I believe this change should be in CPython. Tracking down this bug into the 
Python core took a substantial amount of time and work. Can I do this in my 
Python application and achieve the same effect? Absolutely, but I would argue 
that I shouldn't have to. The fix in CPython is very minimal:

 - It won't break existing code. Even if my project already has a lock around 
calls to Popen, my code will continue to work. Sure, it's a duplicate lock, but 
nothing will break or deadlock.

 - The performance impact is negligible. The lock is only instantiated for 
Windows platforms and acquiring/releasing a lock appears to have very low 
overhead.

 - Because the patch is very small, it can be easily backported to other Python 
versions. As far as I can tell, Python 3.5, 3.4, 3.3, and 2.7 will work with 
the attached patch. The one difference between the Python versions is that 
subprocess.mswindows was renamed to subprocess._mswindows at some point, which 
is very minor.

 - Python 3.4's  new OS-specific inheritable functions [os.get_inheritable(), 
os.set_inheritable(), etc] will continue to work as expected. So, nothing is 
broken or changed there.

Old code won't break, new code will benefit immediately, and existing code will 
work better.


Similar Issues
==

 - #19809: subprocess should warn uses on race conditions when multiple threads 
spawn child processes

 - #2320: Race condition in subprocess using stdin

 - #12739: read stuck with multithreading and simultaneous subprocess.Popen

--
components: Library (Lib)
files: win32-popen-lock.patch
keywords: patch
messages: 248961
nosy: Adam Meily
priority: normal
severity: normal
status: open
title: Windows: subprocess.Popen: race condition for leaking inheritable handles
type: behavior
versions: Python 2.7, Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 3.6
Added file: http://bugs.python.org/file40219/win32-popen-lock.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24909
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[R. David Murray]

R. David Murray added the comment:

Is it possible to construct a test for this?

--
components: +Windows
nosy: +gregory.p.smith, paul.moore, r.david.murray, steve.dower, tim.golden, 
zach.ware
stage:  - patch review
versions:  -Python 3.2, Python 3.3

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24909
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[Adam Meily]

Adam Meily added the comment:

Ok, I can re-implement the patch to meet what you all are looking for. I just 
want to double check that I'm on the same page:

I'll get rid of the lock, because the fix should really be done in the call to 
CreateProcessW. I imagine that I'll be editing Modules/_winapi.c:824. 
Essentially, the fix will include switching _winapi.c to use a STARTUPINFOEX 
structure, and manually passing in the three handles for stdin, stdout, and 
stderr to PROC_THREAD_ATTRIBUTE_HANDLE_LIST.

The one potential problem that I see is that it looks like specifying 
PROC_THREAD_ATTRIBUTE_HANDLE_LIST means that no other handles are inherited, 
even if they are inheritable. Doesn't this break applications that are 
explicitly creating inheritable handles and expecting them to be accessible 
within the subprocess? I could add Windows support for pass_fds, but then 
existing applications would have to be updated to use the argument on Windows.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24909
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[STINNER Victor]

STINNER Victor added the comment:

Yeah, when stdin, stdout or stderr is a pipe, subprocess.Popen() calls 
CreateProcess() with the STARTF_USESTDHANDLES flag and bInheritHandles=TRUE.

This issue was fixed on UNIX for file descriptor inheritance. The fix is a 
major change: Python 3.4 now only creates non-inheritable file descriptors by 
default. See the PEP 446.

For your issue, see the Only Inherit Some Handles on Windows section:
https://www.python.org/dev/peps/pep-0446/#only-inherit-some-handles-on-windows

But this section is not fixed by the PEP. I was decided to fix inheritance of 
file descriptors and inheritance of handles separatly.

I guess that this issue is a duplicate of the issue #19575.

I'm against the idea of adding a lock inside the subprocess module. I may 
introduce deadlock issues. You can already put such lock in your application, 
to call subprocess.Popen (directly or indirectly).

 Currently, the Popen constructor will duplicate any stdout, stdin, and/or 
 stderr handle passed in and make them inheritable, by calling 
 DuplicateHandle. If two threads call Popen at the same time, the newly 
 created inheritable handles will leak into the subprocess that's running 
 being created in the opposite thread. This has consequences when two or more 
 subprocesses are piped together and executed at the time time.

This is a race condition really specific to the subprocess module. Usually, 
handles are created non-inhertable on Windows, so calling CreateProcess() with 
bInheritHandles=TRUE is not an issue in the common case. Here the problem is 
that subprocess makes duplicated handles inheritable. There is a short window 
where a second thread can spawn a process and inherit these handles too.

The real fix is to never make duplicated handles inheritable, but instead to 
use the new PROC_THREAD_ATTRIBUTE_HANDLE_LIST structure. Another option to fix 
the issue is to use a wrapper application and send handles from the parent to 
the child, but this option introduces a lot of complexity since the parent has 
to handle two processes, not only one! Again, see the issue #19575.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24909
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[Adam Meily]

Adam Meily added the comment:

@r.david.murray: Yes I could make a test.

@haypo:

I did not know about the PROC_THREAD_ATTRIBUTE_HANDLE_LIST structure, thanks 
for the heads up. You pointed me in the right direction, and I see now that 
you've been following this, and similar, subprocessing issues on Windows for 
some time.

I understand the hesitancy for adding a Lock to the standard library. Unless I 
am mistaken, I don't think my patch could cause a deadlock. I am releasing the 
lock in a `finally` block. So, the way I see it, the only way a deadlock can 
occur is if the thread crashes while in C code, which will cause the `finally` 
block to not execute. The lock will stay acquired, and then another thread will 
deadlock trying to acquire the lock. However, if that is the case and the 
thread crashed in C, the whole interpreter instance would have crashed, not 
just the Thread, which would make the deadlock impossible.

It looks like the structure you reference to, PROC_THREAD_ATTRIBUTE_HANDLE_LIST 
(STARTUPINFOEX), is only available in Windows Vista and after. Which means that 
Windows XP/Server 2003 would still have this issue.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24909
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[STINNER Victor]

STINNER Victor added the comment:

Yeah, when stdin, stdout or stderr is a pipe, subprocess.Popen() calls 
CreateProcess() with the STARTF_USESTDHANDLES flag and bInheritHandles=TRUE.

This issue was fixed on UNIX for file descriptor inheritance. The fix is a 
major change: Python 3.4 now only creates non-inheritable file descriptors by 
default. See the PEP 446, especially the Only Inherit Some Handles on Windows 
section:
https://www.python.org/dev/peps/pep-0446/#only-inherit-some-handles-on-windows

This part is out of the scope of the PEP.

For me, the real fix would be to create non-inheritable

I guess that this issue is a duplicate of the issue #19575.

I'm again the idea of adding a lock inside the subprocess module. I may add 
deadlocks. You can already put this lock in your application, before calling 
subprocess.Popen (directly or indirectly).

 Currently, the Popen constructor will duplicate any stdout, stdin, and/or 
 stderr handle passed in and make them inheritable, by calling 
 DuplicateHandle. If two threads call Popen at the same time, the newly 
 created inheritable handles will leak into the subprocess that's running 
 being created in the opposite thread. This has consequences when two or more 
 subprocesses are piped together and executed at the time time.

This is a race condition specific to the subprocess module. Usually, handles 
are created non-inhertable, so calling CreateProcess() with 
bInheritHandles=TRUE is not an issue.

The real fix is *not* make duplicated handles inheritable, but use the new 
PROC_THREAD_ATTRIBUTE_HANDLE_LIST structure. Again, see the issue #19575.

--
nosy: +haypo

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24909
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[STINNER Victor]

STINNER Victor added the comment:

 It looks like the structure you reference to, 
 PROC_THREAD_ATTRIBUTE_HANDLE_LIST (STARTUPINFOEX), is only available in 
 Windows Vista and after. Which means that Windows XP/Server 2003 would still 
 have this issue.

Windows XP is no more supported in Python 3.5:
https://docs.python.org/dev/whatsnew/3.5.html#unsupported-operating-systems

For Windows Server 2003, yes, we will have to keep the current code which has 
the race condition. We did the same in the PEP 446 to clear the inherit flag. 
It's atomic or not depending on the function, on the operating system and even 
depending on the operating system version. So the PEP 446 doesn't fix all race 
conditions on old operating systems.

https://www.python.org/dev/peps/pep-0446/#atomic-creation-of-non-inheritable-file-descriptors

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24909
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[STINNER Victor]

Changes by STINNER Victor victor.stin...@gmail.com:


--
Removed message: http://bugs.python.org/msg248964

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24909
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24909] Windows: subprocess.Popen: race condition for leaking inheritable handles

[Zachary Ware]

Zachary Ware added the comment:

STINNER Victor added the comment:
 For Windows Server 2003, yes, we will have to keep the current code which has 
 the race condition.

Server 2003 is also unsupported in 3.5+ (MS extended support ended in July).

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24909
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com