Repository navigation
Danger of leaking file descriptors when spawning processes #12148
Description
We're not opening anything with CLOEXEC, so we're in theory leaking file descriptors across forks (they stay alive as long as the child stays alive).
We currently use getdtablesize to close all these descriptors, but as #12103 (comment) says, this isn't enough if a thread manually lowers RLIMIT_NOFILE.
We should consider opening file descriptors wherever possible with CLOEXEC, but this also sounds like it's a tricky situation (not always supported to specify the flag at open-time).
Activity
ben0x539 commentedon Feb 21, 2014
... but what if a forked process wants to hang on to existing file descriptors?
alexcrichton commentedon Apr 28, 2014
#13790 has example code triggering this problem as well as some solutions for some platforms.
thestinger commentedon Sep 19, 2014
Opening all file descriptors as CLOEXEC should be the default and can be fully implemented on both Linux and Windows. I don't know if this can be done with absolutely zero race conditions on OS X and FreeBSD (atomic operations rather than setting it after opening), but it's probably possible. This is a major security issue and has backwards compatibility implications.
bnoordhuis commentedon Sep 19, 2014
FreeBSD 10, like Linux >= 26.27, has atomic O_CLOEXEC system calls. For other POSIX platforms, I imagine you could manually clean up in a pthread_atfork() handler, though that won't work for file descriptors > RLIMIT_NOFILE.
OS X has a POSIX_SPAWN_CLOEXEC_DEFAULT posix_spawnattr_t flag but that only helps when spawning a new process, not when you want to fork. You use posix_spawn_file_actions_addinherit_np() for file descriptors that the new process should inherit but that is complicated by the fact that it only accepts file descriptors < OPEN_MAX (10,240.)
pnkfelix commentedon Sep 25, 2014
this issue should be considered blocking the stabilization of the relevant API's.
The relevant API's are not yet considered stable (they are currently
experimental).So, assigning P-high, not 1.0.
alexcrichton commentedon Mar 5, 2015
Closing in favor of rust-lang/rfcs#941
thestinger commentedon Mar 5, 2015
This is a real, serious security issue and needs to be fixed regardless of whether a proposed redesign happens. Closing the issue doesn't change the fact that it's getting a CVE assigned if it's not fixed by 1.0.
aturon commentedon Mar 5, 2015
For the record, I'm worried about closing this out without a clear resolution for 1.0.
While I think the APIs for the new IO system are in good shape (and are quite conservative), I think we should take an additional pass through the implementation looking for issues like this, and try to document as much of the details as possible (i.e. what syscalls a given API maps to, and what promises we're making behavior-wise).
pcwalton commentedon Mar 10, 2015
I worry people will rely on calling
libc::fork()and we will not be able to change the behavior backwards compatibly. I was bitten by this in gaol so it is not a theoretical concern. I will nominate this.224 remaining items
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()
BACKPORT: open: add close_range()