Danger of leaking file descriptors when spawning processes

[alexcrichton]

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).

[ben0x539]

... but what if a forked process wants to hang on to existing file descriptors?

[alexcrichton]

#13790 has example code triggering this problem as well as some solutions for some platforms.

[thestinger]

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]

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]

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]

Closing in favor of rust-lang/rfcs#941

[thestinger]

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]

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]

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.

[thestinger]

They leak whether or not you call fork because a loop up to the rlimit isn't enough. There's no guarantee that the file descriptors are allocated sequentially up to that limit - and in practice they won't be if a limit was applied even shortly after starting. It could also need to issue a million system calls as it's based on the assumption that the rlimit...

[l0kod]

To explicitly list all open file descriptors, we can replace the getdtablesize call with a listing of /proc/self/fd/* when available. This should be useful for external lib/FFI leaking file descriptors or for explicit fork (without execve) or if Rust miss something.
Moreover, this closing loop should not discriminate file descriptor by their number.

Anyway, O_CLOEXEC should be used everywhere in Rust.

cc #23233
cc #22797

[l0kod]

To explicitly list all open file descriptors, we can replace the getdtablesize call with a listing of /proc/self/fd/* when available.

This will also help to reduce the syscall storm when using Process::spawn :)

[pnkfelix]

1.0, P-backcompat-libs, I-needs-decision.

[DemiMarie]

I believe that, where possible, the FD_CLOEXEC flag should be passed to open, and that calls like pipe2 (which allow the flag to be set atomically) should be used where available. When this is not possible, FD_CLOEXEC should still be set as soon as possible.

Doing a bunch of calls to close(2) would be a severe performance penalty on every fork()/exec() for systems that lack procfs. It also would not prevent the FD leak for processes created outside of Rust.

This does mean bypassing C's stdio.

[thestinger]

When this is not possible, FD_CLOEXEC should still be set as soon as possible.

It's always possible to avoid leaks without the loop hack, at least on Linux and Windows. It's not an unsolved problem.

[DemiMarie]

I know that it is always possible.

I am just saying that Rust should still set the FD_CLOEXEC flag, even when it is not necessary to avoid leaks to processes created by Rust, for the benefit of processes not created by Rust, but by other libraries in the same process. If the FD_CLOEXEC flag was not used, all file descriptors would be leaked to these processes. If it is set, none would be.

This is especially important when a Rust library exposes a C API, as the C program may not be aware of the FD creation in Rust code.

I hope that I am not belaboring an already-made point -- please let me know if I am. I guess I am emphatically agreeing with I0kod.

[thestinger]

I'm stating that it's always possible to set CLOEXEC along with the Windows equivalent and it should always be set. There's no reason to resort to a non-atomic combination of a call creating a file descriptor and fcntl on these platforms, and no reason to resort to the looping hack.

[DemiMarie]

On Mon, 2015-03-16 at 22:03 -0700, Daniel Micay wrote:

I'm stating that it's always possible to set CLOEXEC along with the
Windows equivalent and it should always be set. There's no reason to
resort to a non-atomic combination of a call creating a file
descriptor and fcntl on these platforms, and no reason to resort to
the looping hack.

—
Reply to this email directly or view it on GitHub.

Ah okay. Then we agree completely.

[pnkfelix]

nominating for future triage next week.

[aturon]

Note: @alexcrichton and I discussed this at length today, and believe that the way forward is to indeed set CLOEXEC, to do so atomically whenever possible (not possible on all platform versions in all scenarios), and to forgo the close loop when spawning processes. We can clearly document the behavior of std APIs that open file descriptors to specify under what platform/versions CLOEXEC is set atomically.

[aturon]

We should move using CLOEXEC before 1.0, so suggesting that nomination leave the labels/milestone as-is.