Repository navigation
subprocess close_fds behavior should only close open fds #52300
Description
| BPO | 8052 |
|---|---|
| Nosy | @loewis, @gpshead, @pitrou, @vstinner, @socketpair |
| Files |
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 = 'https://github.com/gpshead'
closed_at = <Date 2012-01-22.06:27:53.761>
created_at = <Date 2010-03-04.03:17:56.815>
labels = ['type-bug']
title = 'subprocess close_fds behavior should only close open fds'
updated_at = <Date 2014-03-19.14:49:11.698>
user = 'https://github.com/gpshead'bugs.python.org fields:
activity = <Date 2014-03-19.14:49:11.698>
actor = 'vstinner'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2012-01-22.06:27:53.761>
closer = 'gregory.p.smith'
components = []
creation = <Date 2010-03-04.03:17:56.815>
creator = 'gregory.p.smith'
dependencies = []
files = ['24255', '24256']
hgrepos = []
issue_num = 8052
keywords = ['patch']
message_count = 31.0
messages = ['100373', '125278', '125285', '125361', '125388', '125389', '132297', '132303', '132307', '132308', '132318', '132355', '132530', '132532', '132573', '132669', '151284', '151285', '151286', '151292', '151420', '151421', '151746', '151750', '151752', '151761', '151762', '214095', '214096', '214098', '214099']
nosy_count = 12.0
nosy_names = ['loewis', 'gregory.p.smith', 'pitrou', 'vstinner', 'ferringb', 'jyasskin', 'nadeem.vawda', 's7v7nislands', 'neologix', 'rosslagerwall', 'socketpair', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'needs patch'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue8052'
versions = ['Python 3.1', 'Python 2.7', 'Python 3.2']
Activity
gpshead commentedon Mar 4, 2010
The subprocess module's close_fds=True behavior is suboptimal. It gets the maximum fd number and calls close() on each possible fd in the child process.
When you're running on large systems configured for 1million+ file descriptor limits per process.... This makes launching a subprocess uber painful.
I'm in the middle of some posix subprocess fixing, i'll add this to my todo list and fix it up.
pitrou commentedon Jan 4, 2011
Is it still a problem now that there's a C path?
Furthermore, how do you plan to get a list of open fds?
gpshead commentedon Jan 4, 2011
its still a problem, even the C path has to call close() a million times in that case. thats a huge number of wasted syscalls. fixing this is blocking on a good way to get the list of open fds.
I have seen other subprocess code do it using the race condition method of having the parent process get the list from os.listdir('/prod/self/fds') but that isn't guaranteed to get everything due to the race and we I don't believe we have enough posix async signal safe syscalls to do that in between the fork+exec.
I heard talk of actual system calls to do this type of thing (getting a list of a processes open fds) being added to future Linux kernels but I have not followed up on that to see if any have or if they are what we need.
rosslagerwall commentedon Jan 4, 2011
Does this mean that it's better to call the close() syscall 1000 or 1000 0000 times rather than listing the open fds & closing say a handful?
On Linux, the listdir function results in an open() syscall, 2 or so getdents() calls and a close() call - could this not be done inbetween the fork & exec, even if the actual python function isn't called directly?
nadeemvawda commentedon Jan 5, 2011
According to POSIX [1], if a multi-threaded program calls fork(), the child process may only use async-signal-safe system calls between fork() and exec*(). readdir() is not required to be async-safe [2], so reading /proc/self/fds in the child process is undefined behaviour. This is a pity, since it would IMO be a much cleaner solution than the current code.
Of course, procfs isn't standard in any case; would it be necessary to have a fallback for systems without it? Or do all *nix systems that we care about provide it? In the former case, I suppose it might be possible to use the procfs on systems where readdir() is known to be safe, and use the fallback where it isn't. But such special cases would complicate the code rather than simplifying it...
[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/fork.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html
pitrou commentedon Jan 5, 2011
/proc/self doesn't exist under OpenSolaris. You have to use /proc/<pid>.
And I suppose there can be circumstances where /proc isn't available at
all (chroot perhaps?). So we would definitely have to provide a
fallback.
rosslagerwall commentedon Mar 27, 2011
See bpo-11284 (a duplicate) for more discussion about this issue.
vstinner commentedon Mar 27, 2011
Can we use FD_CLOEXEC to archive this goal? Example: open all files with FD_CLOEXEC set and don't close explicitly files on fork. preexec_fn will get access to the files, but the problem is exec(), not preexec_fn.
I suppose that it will slow down programs not using subprocess, and it may have border effects.
neologix commentedon Mar 27, 2011
If you're suggesting to set FDs CLOEXEC by default, I think it's neither possible nor reasonable:
As for the dedicated syscalls, there's already been some discussion about closefrom and friends, but Gregory did some research and it looked like those are not async-safe - which, if it's really the case, renders those calls mostly useless.
neologix commentedon Mar 27, 2011
Ooops, it's of course not going to break code containing accept + fork or pipe + fork, you obviously also need an execve ;-)
But the point is that you can't change the semantics of FDs being inheritable across an execve (think about inetd for example).
rosslagerwall commentedon Mar 27, 2011
I don't think setting the cloexec flag is a viable solution, especially since fds can be opened in custom c modules.
For what its worth, an strace of Java's Process class appears to "cheat" by opening /proc/self/fd inbetween fork & exec.
loewis commentedon Mar 27, 2011
I think we should use FD_CLOEXEC in all places where it's reasonable.
As others have pointed out, we shouldn't set FD_CLOEXEC for file
descriptors where the application hasn't explicitly requested that.
nadeemvawda commentedon Mar 29, 2011
Looking at the OpenJDK source confirms this:
http://hg.openjdk.java.net/jdk6/jdk6/jdk/file/tip/src/solaris/native/java/lang/UNIXProcess_md.c
I wonder whether the Java people are simply unaware of the potential problem?
Or perhaps they have checked the Linux and Solaris implementations of readdir()
and confirmed that it is in fact safe on those platforms. Even if this is the
case, I would be wary of doing things the same way - there's no guarantee that
the implementation won't change out from underneath us.
I like the approach suggested by Gregory in bpo-11284 (msg129912) - if we are
sure the program is not multithreaded, get the list of fds before forking, and
use that; otherwise, do it the slow way.
On Linux, the multithreading check could be implemented by checking the number
of subdirectories in /proc/<pid>/task/. I don't know if *BSD (or other Unices)
have a similar way to do this. Some quick Googling seems to indicate that
FreeBSD, at least, doesn't make this information available through the procfs
(which is apparently deprecated anyway). Could someone with wider *nix
knowledge chip in here?
5 remaining items
ferringb commentedon Jan 15, 2012
Unless the OS gives some way to optimize the process (whether inferring from procfs, or making use of spawn_closefrom), there really isn't anything we can do. O_CLOEXEC is one option, but that's basically the same as the close loop in terms of syscalls- specifically post fork looping over the range and setting it. Beyond that, it's linux specific, so only would be used if the root python was invoked from lacked procfs.
I'm willing to extend my original patch to handle alternate OS hints as needed; in the same way, the nlinks trick I can implement although I'd be more inclined to just limit my original closerange patch to OSs that have a sane opendir and procfs.
rosslagerwall commentedon Jan 15, 2012
FreeBSD has a /dev/fd as well as a procfs (deprecated AFAIK).
However, both may not be mounted so a patch would *need* to at least fallback to the current functionality.
gpshead commentedon Jan 17, 2012
attaching a patch that implements this safely.
gpshead commentedon Jan 17, 2012
attaching a better formatted one for review.
python-dev commentedon Jan 21, 2012
New changeset 61aa484a3e54 by Gregory P. Smith in branch '3.2':
Fixes issue bpo-8052: The posix subprocess module's close_fds behavior was
http://hg.python.org/cpython/rev/61aa484a3e54
New changeset 8879874d66a2 by Gregory P. Smith in branch 'default':
Fixes issue bpo-8052: The posix subprocess module's close_fds behavior was
http://hg.python.org/cpython/rev/8879874d66a2
python-dev commentedon Jan 21, 2012
New changeset 780992c9afea by Gregory P. Smith in branch '3.2':
Add a Misc/NEWS entry for bpo-8052.
http://hg.python.org/cpython/rev/780992c9afea
New changeset 1f0a01dc723c by Gregory P. Smith in branch 'default':
A Misc/NEWS entry for bpo-8052.
http://hg.python.org/cpython/rev/1f0a01dc723c
python-dev commentedon Jan 21, 2012
New changeset d0acd8169c2a by Gregory P. Smith in branch '3.2':
Bugfix for issue bpo-8052 fix on *BSD variants.
http://hg.python.org/cpython/rev/d0acd8169c2a
New changeset 5be3dadd2eef by Gregory P. Smith in branch '3.2':
Another issue bpo-8052 bugfix (related to previous commit).
http://hg.python.org/cpython/rev/5be3dadd2eef
New changeset e52d81e0c750 by Gregory P. Smith in branch 'default':
bugfix for bpo-8052 fixes on *BSD platforms.
http://hg.python.org/cpython/rev/e52d81e0c750
python-dev commentedon Jan 22, 2012
New changeset 754c2eb0a92c by Gregory P. Smith in branch '3.2':
Fix FreeBSD, NetBSD and OpenBSD behavior of the issue bpo-8052 fix.
http://hg.python.org/cpython/rev/754c2eb0a92c
New changeset 7d4658a8de96 by Gregory P. Smith in branch 'default':
Fix FreeBSD, NetBSD and OpenBSD behavior of the issue bpo-8052 fix.
http://hg.python.org/cpython/rev/7d4658a8de96
gpshead commentedon Jan 22, 2012
For FreeBSD, Python 3.2 and 3.3 now check to see if /dev/fd is valid. Be sure and "mount -t fdescfs none /dev/fd" on FreeBSD if you want faster subprocess launching. Run a FreeBSD buildbot? Please do it!
For Python 3.1 the fix for bpo-13788 would fix this, but I believe 3.1 is in security fix only mode at this point so we're not going to backport that os.closerange change that far.
socketpair commentedon Mar 19, 2014
Calling getdents()/readdir64() repeatedly while closing descriptors provides unexpected behaviour. Reading directory while it modified is not safe by default. For example: http://en.it-usenet.org/thread/18514/15719/.
So, we should re-open directory if we received full array of structures. I don't know if just lseek(dirfd, 0) sufficies.
Please reopen bug, as Linux behaviour of stable reading /proc/<pid>/fd may be broken in future without any error at python side (!) (typically, second call returns empty list if dir was modified)
vstinner commentedon Mar 19, 2014
"Calling getdents()/readdir64() repeatedly while closing descriptors provides unexpected behaviour."
Please open a new issue.
socketpair commentedon Mar 19, 2014
Also, it is not said in manual if getdents() may be interrupted by signal. Assuming current code, error is not checked, so some (or all) descriptors will be skipped in case of error.
vstinner commentedon Mar 19, 2014
"Also, it is not said in manual if getdents() may be interrupted by signal. Assuming current code, error is not checked, so some (or all) descriptors will be skipped in case of error."
This issue is closed, please open a new issue to discuss that.