subprocess close_fds behavior should only close open fds

[gpshead]

BPO	8052
Nosy	@loewis, @gpshead, @pitrou, @vstinner, @socketpair
Files	subprocess-close-open-fds-gps01.diff subprocess-close-open-fds-gps02.diff

^{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']

[gpshead]

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]

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]

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]

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]

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]

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?

/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]

See bpo-11284 (a duplicate) for more discussion about this issue.

[vstinner]

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]

If you're suggesting to set FDs CLOEXEC by default, I think it's neither possible nor reasonable:

• you have to take into account not only files, but also pipes, sockets, etc
• there's no portable way to e.g. open a file and set it CLOEXEC atomically
• first and foremost, it' going to break a lot of existing code, for example, pipe + fork, accept + fork, etc
  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]

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]

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]

Can we use FD_CLOEXEC to archive this goal?

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]

For what its worth, an strace of Java's Process class appears to "cheat" by
opening /proc/self/fd inbetween fork & exec.

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?