When subprocess.Popen is called with subprocess.PIPE and os.fork() fails (due to insufficient memory for example), the pipes created by _get_handles() are not closed.

Steps to reproduce:

1) Launch a python shell, import subprocess
2) Create a situation where os.fork() will fail (I ran a program to eat up nearly all of the memory on a system)
3) subprocess.Popen('/bin/echo', stdin=subprocess.PIPE)
4) OSError(12, 'Cannot allocate memory')
5) ls /proc/$pid/fd , There are now extra pipes.

I tested on Ubuntu 11.10 python (2.7.2-5ubuntu1).  My reading of the 2.7 and 3.3 development trees suggest that this is an issue with both of those branches, but I don't have a 3.3 installation to confirm with.

I've attached a snippet that fixes it for my version of Python on Ubuntu.  No idea what ramifications it will have for other versions/OS/etc.  No automated testing included because I'm not entirely sure how to replicate this without eating up a ton of ram or doing something naughty with ulimit.

Just read the docs for stdin and stdout.  This patch will indtroduce a bug where a provided file descriptor can be closed.  This definitely shouldn't close a file descriptor that isn't created by _get_handles().  I'll update.

Patch now only closes pipe fds created by Popen

I would catch ALL exceptions, not only "OSError".

An easy way to test this would be to test a subclass of Popen with "_execute_child()" method overrided for always raising an exception.

On Unix the test could just open six fds, close them taking note of the values, call this code forcing an exception, catch it, open six new fds and verify that the numbers are the same. So we verify that neither of the six fds created "inside" are leaked.

What should we do for Windows?


Maybe the easier and more portable approach for exception cleanup would be to do "_execute_child()" AFTER the "fdopen()" dance, so we can just do "close()" if any exception is raised.

Also, the cleanup MUST be done ONLY if the fds were created inside the function (PIPE), not if the fd came from the caller.

python 3 already catches all exceptions and handles closing of p2cwrite, c2pread and errread here.  i don't know which branch this patch is against.  Regardless, it makes sense that the other fd's, if created by us, also need to be cleaned up.  The code also needs to ignore exceptions from the close() call.

 http://hg.python.org/cpython/file/cbdd6852a274/Lib/subprocess.py#l811

attachment against 2.7

if I understand this code correctly, the fix shouldn't need to be fixed separately on windows and Linux, since the thing handled by init is just a file descriptor. 

Good idea on the testing. I'll give that a shot tomorrow. I think 3.3 will need some extra cleanup too.

The cleanup code in python 3 validates my idea of simplifying cleanup moving "_execute_child()" after the platform specific code.

I wonder what "raise" will actually raise if this cleanup code catches & ignores "close()" exception :-).

Mark, could you consider to fill&send a contributor form agreement? http://www.python.org/psf/contrib/

In fact, nested exception management in python 2 and python 3 actually diverges. BEWARE: (Python 3 does the right thing, once again :-)

"""
Python 2.7.3 (default, Apr 12 2012, 13:11:53) 
[GCC 4.4.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> try :
...   1/0
... except :
...   try :
...     raise RuntimeError("TEST")
...   except :
...     pass
...   raise
... 
Traceback (most recent call last):
  File "<stdin>", line 5, in <module>
RuntimeError: TEST
"""

"""
Python 3.3.0 (default, Oct  2 2012, 02:07:16) 
[GCC 4.4.3] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> try :
...   1/0
... except :
...   try :
...     raise RuntimeError("TEST")
...   except :
...     pass
...   raise
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
ZeroDivisionError: division by zero
"""

http://stackoverflow.com/questions/8997431/is-there-any-way-to-access-nested-or-re-raised-exceptions-in-python

"""
This is known as Exception Chaining and is suported in Python 3.

PEP 3134: http://www.python.org/dev/peps/pep-3134/

In Python 2, the old exception is lost when you raise a new one, unless you save it in the except block.
"""

Python2 management should be something like:

"""
Python 2.7.3 (default, Apr 12 2012, 13:11:53) 
[GCC 4.4.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> try :
...   1/0
... except BaseException as e :
...   try :
...     raise
...   finally :
...     try :
...       raise RuntimeError("TEST")
...     except :
...       pass
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
ZeroDivisionError: integer division or modulo by zero
"""

Sorry, UGLY.

Idea from http://www.doughellmann.com/articles/how-tos/python-exception-handling/index.html

Replace "except BaseException as e :" with just "except:". It was a remnant of my tests.

> No automated testing included because I'm not entirely sure how to replicate this without eating up a ton of ram or doing something naughty with ulimit.

Simply use RLIMIT_NPROC, from a subprocess:
"""
$ cat /tmp/test.py
import subprocess

ps = [ ]

for i in range(1024):
    p = subprocess.Popen(['sleep', '10'])
    ps.append(p)
$ python /tmp/test.py
Traceback (most recent call last):
  File "/tmp/test.py", line 7, in ?
    p = subprocess.Popen(['sleep', '10'])
  File "/usr/lib64/python2.4/subprocess.py", line 550, in __init__
    errread, errwrite)
  File "/usr/lib64/python2.4/subprocess.py", line 919, in _execute_child
    self.pid = os.fork()
OSError: [Errno 11] Resource temporarily unavailable
$ ulimit -u 1024
"""

Not POSIX, but supported by Linux and BSD, which should be enough.

The problem with monkey-ptching is that you don't test the real
codepath, and it may break in a future version (especially since here
it would be using a preivate API).

Also, I didn't check, but if the problems also occurs on execve()
failure, then it's much simpler: simply call Popen() with an
invalid/nonexisting executable.

The problem with using RLIMIT is that the testsuite could be executing several tests in parallel using independent threads, for instance. You don't want to influence unrelated tests.

Overiding private methods is ugly, but if the code evolves the test would break, and the programmer just have to update it. I think that "sometimes" we have to be "practical". There are plenty of examples of this in the testsuite, using implementation details, etc.

> The problem with using RLIMIT is that the testsuite could be executing several tests in parallel using independent threads, for instance. You don't want to influence unrelated tests.

That's why I suggested to run it in a subprocess: this is used
frequently, e.g. for signal or deadlock tests.

Doesn't exhibit when execve fails, because by the time execve has been reached we've closed the pipes that we're supposed to close on the parent process, and the pipes that are meant to remain open on the parent process get caught by existing cleanup code.  It's unfortunately got to fail somewhere in the vicinity of the fork to fake it using the actual _execute_child.

Stubbing _execute_child out for a test is easiest. No need to craft ways to
cause an actual fork failure.

Patch fixes and tests fd leak on Python 3.3.  Test fails without fix, passes with fix.

I found an existing test looking for fd leaks for another bug.  Borrowed the verification bits from it.

There were some other test failures when I ran the subprocess suite on my laptop, but it more like I had some environmental issue rather than having genuinely broken anything.  If somebody else (or the test bots?) could run the tests I would appreciate it.

Here's more or less the same fix and test on 2.7.  I jumped through the hoop to preserve the original exception and traceback even if os.close() raises an exception.

This follows the 3.3 branch's cleanup behavior of silently suppressing errors in the cleanup code.

I've also submitted the contributor form requested.

My contributor form has been accepted.  Anything else I should be doing to work towards getting a fix applied?

Thanks! I'm looking into applying these tonight (including 3.2) with a couple minor edits.

New changeset 63ff4c9a2ed2 by Gregory P. Smith in branch '3.2':
Fixes issue #16327: The subprocess module no longer leaks file descriptors
http://hg.python.org/cpython/rev/63ff4c9a2ed2

New changeset a6a6c349af7e by Gregory P. Smith in branch '3.3':
Fixes issue #16327: The subprocess module no longer leaks file descriptors
http://hg.python.org/cpython/rev/a6a6c349af7e

New changeset a9e238168588 by Gregory P. Smith in branch 'default':
Fixes issue #16327: The subprocess module no longer leaks file descriptors
http://hg.python.org/cpython/rev/a9e238168588

New changeset e67620048d2f by Gregory P. Smith in branch '2.7':
Fixes issue #16327: The subprocess module no longer leaks file descriptors
http://hg.python.org/cpython/rev/e67620048d2f

New changeset 2bdd984a55ac by Gregory P. Smith in branch '2.7':
Fix issue #16140 bug that the fix to issue #16327 added - don't double
http://hg.python.org/cpython/rev/2bdd984a55ac