classification
Title: subprocess.Popen leaks file descriptors on os.fork() failure
Type: resource usage Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Mark.Gius, gregory.p.smith, jcea, neologix, python-dev
Priority: normal Keywords: patch

Created on 2012-10-25 22:31 by Mark.Gius, last changed 2012-11-11 10:02 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
fd_leak_fix.diff Mark.Gius, 2012-10-25 22:31 Naive fix for bug review
fd_leak_fix_v2.diff Mark.Gius, 2012-10-25 23:10 Less buggy naive fix review
fix_16327_on_3.3.diff Mark.Gius, 2012-10-29 18:24 Fix 16327 on 3.3 review
fix_16327_on_2.7.diff Mark.Gius, 2012-10-29 18:29 Fix 16327 on 2.7 review
Messages (26)
msg173804 - (view) Author: Mark Gius (Mark.Gius) * Date: 2012-10-25 22:31
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.
msg173807 - (view) Author: Mark Gius (Mark.Gius) * Date: 2012-10-25 22:59
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.
msg173808 - (view) Author: Mark Gius (Mark.Gius) * Date: 2012-10-25 23:10
Patch now only closes pipe fds created by Popen
msg173810 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-26 00:16
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.
msg173811 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-10-26 00:19
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
msg173813 - (view) Author: Mark Gius (Mark.Gius) * Date: 2012-10-26 00:42
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.
msg173814 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-26 00:48
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 :-).
msg173815 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-26 00:49
Mark, could you consider to fill&send a contributor form agreement? http://www.python.org/psf/contrib/
msg173816 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-26 01:18
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
"""
msg173817 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-26 01:24
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.
"""
msg173818 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-26 01:34
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
msg173819 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-26 01:36
Replace "except BaseException as e :" with just "except:". It was a remnant of my tests.
msg173821 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-10-26 06:04
> 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).
msg173825 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-10-26 07:30
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.
msg173832 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-26 10:37
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.
msg173837 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-10-26 12:11
> 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.
msg174085 - (view) Author: Mark Gius (Mark.Gius) * Date: 2012-10-28 22:50
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.
msg174098 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-10-29 02:08
Stubbing _execute_child out for a test is easiest. No need to craft ways to
cause an actual fork failure.
msg174137 - (view) Author: Mark Gius (Mark.Gius) * Date: 2012-10-29 18:24
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.
msg174138 - (view) Author: Mark Gius (Mark.Gius) * Date: 2012-10-29 18:29
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.
msg174139 - (view) Author: Mark Gius (Mark.Gius) * Date: 2012-10-29 18:30
I've also submitted the contributor form requested.
msg174940 - (view) Author: Mark Gius (Mark.Gius) * Date: 2012-11-05 20:44
My contributor form has been accepted.  Anything else I should be doing to work towards getting a fix applied?
msg175324 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-11-11 05:50
Thanks! I'm looking into applying these tonight (including 3.2) with a couple minor edits.
msg175326 - (view) Author: Roundup Robot (python-dev) Date: 2012-11-11 06:34
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
msg175327 - (view) Author: Roundup Robot (python-dev) Date: 2012-11-11 06:49
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
msg175339 - (view) Author: Roundup Robot (python-dev) Date: 2012-11-11 10:02
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
History
Date User Action Args
2012-11-11 10:02:09python-devsetmessages: + msg175339
2012-11-11 06:50:51gregory.p.smithsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2012-11-11 06:49:16python-devsetmessages: + msg175327
2012-11-11 06:34:26python-devsetnosy: + python-dev
messages: + msg175326
2012-11-11 05:50:53gregory.p.smithsetmessages: + msg175324
2012-11-05 20:44:37Mark.Giussetmessages: + msg174940
2012-10-29 18:30:41Mark.Giussetmessages: + msg174139
2012-10-29 18:29:05Mark.Giussetfiles: + fix_16327_on_2.7.diff

messages: + msg174138
2012-10-29 18:24:27Mark.Giussetfiles: + fix_16327_on_3.3.diff

messages: + msg174137
2012-10-29 02:08:51gregory.p.smithsetmessages: + msg174098
2012-10-28 22:50:49Mark.Giussetmessages: + msg174085
2012-10-26 12:11:18neologixsetmessages: + msg173837
2012-10-26 10:37:28jceasetmessages: + msg173832
2012-10-26 07:30:05neologixsetmessages: + msg173825
2012-10-26 06:04:14neologixsetmessages: + msg173821
2012-10-26 01:36:23jceasetmessages: + msg173819
2012-10-26 01:34:56jceasetmessages: + msg173818
2012-10-26 01:24:31jceasetmessages: + msg173817
2012-10-26 01:18:07jceasetmessages: + msg173816
2012-10-26 00:49:32jceasetmessages: + msg173815
2012-10-26 00:48:10jceasetmessages: + msg173814
2012-10-26 00:42:29Mark.Giussetmessages: + msg173813
2012-10-26 00:19:13gregory.p.smithsetmessages: + msg173811
2012-10-26 00:16:09jceasetnosy: + jcea
messages: + msg173810
2012-10-26 00:01:31gregory.p.smithsetassignee: gregory.p.smith
2012-10-25 23:10:02Mark.Giussetfiles: + fd_leak_fix_v2.diff

messages: + msg173808
2012-10-25 22:59:30Mark.Giussetmessages: + msg173807
2012-10-25 22:32:12pitrousetnosy: + gregory.p.smith, neologix
stage: patch review

versions: + Python 3.2, Python 3.3, Python 3.4
2012-10-25 22:31:03Mark.Giuscreate