New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
subprocess.Popen leaks file descriptors on os.fork() failure #60531
Comments
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:
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
""" |
""" 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. |
Simply use RLIMIT_NPROC, from a 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 |
Also, I didn't check, but if the problems also occurs on execve() |
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. |
That's why I suggested to run it in a subprocess: this is used |
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 |
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': New changeset a6a6c349af7e by Gregory P. Smith in branch '3.3': New changeset a9e238168588 by Gregory P. Smith in branch 'default': |
New changeset e67620048d2f by Gregory P. Smith in branch '2.7': |
New changeset 2bdd984a55ac by Gregory P. Smith in branch '2.7': |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: