Skip to content
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

Leverage atomically setting CLOEXEC where possible #24237

Closed
5 tasks done
alexcrichton opened this issue Apr 9, 2015 · 2 comments
Closed
5 tasks done

Leverage atomically setting CLOEXEC where possible #24237

alexcrichton opened this issue Apr 9, 2015 · 2 comments

Comments

@alexcrichton
Copy link
Member

After #24034 lands we will be setting CLOEXEC on all file descriptors on unix in the standard library. Currently we do so in a nonatomic fashion, however, so it's possible to continue to leak file descriptors into children if a thread is concurrently forking.

Many platforms have methods of creating CLOEXEC file descriptors atomically, but many of the APIs are quite new and they need to be properly detected at runtime. For example, the current instances of fd.set_cloexec() that need to be migrated are listed below. A reminder of our minimum platform requirements are linux 2.6.18 and OSX 10.7.

The hard part about this bug is figuring out if it's possible to avoid compile-time detection of these functions (and instead rely on runtime detection).

@l0kod
Copy link
Contributor

l0kod commented Apr 10, 2015

cc #24307

tbu- added a commit to tbu-/rust that referenced this issue Aug 24, 2015
On Linux the flag is just ignored if it is not supported:
https://lwn.net/Articles/588444/

Touches rust-lang#24237.
bors added a commit that referenced this issue Aug 25, 2015
On Linux the flag is just ignored if it is not supported:
https://lwn.net/Articles/588444/

Still needs the values of O_CLOEXEC on the BSDs.

Touches #24237.
bors added a commit that referenced this issue Aug 31, 2015
Still needs values of F_DUPFD_CLOEXEC on other OSs.

For Bitrig, NetBSD and OpenBSD the constant was incorrectly in posix01, when
it's actually posix08. In order to maintain backwards-compatiblity, the
constant was only copied, not moved.

cc #24237
bors added a commit that referenced this issue Feb 6, 2016
These commits finish up closing out #24237 by filling out all locations we create new file descriptors with variants that atomically create the file descriptor and set CLOEXEC where possible. Previous support for doing this in `File::open` was added in #27971 and support for `try_clone` was added in #27980. This commit fills out:

* `Socket::new` now passes `SOCK_CLOEXEC`
* `Socket::accept` now uses `accept4`
* `pipe2` is used instead of `pipe`

Unfortunately most of this support is Linux-specific, and most of it is post-2.6.18 (our oldest supported version), so all of the detection here is done dynamically. It looks like OSX does not have equivalent variants for these functions, so there's nothing more we can do there. Support for BSDs can be added over time if they also have these functions.

Closes #24237
@ioquatix
Copy link

I know this is closed, but I just have to comment on how well thought out and organised the original description was - how clearly the problem is captured and then it's solution tracked. Well done everyone.

krallin added a commit to krallin/perf-event that referenced this issue Mar 10, 2023
Note: this is a behavior change.

Currently, this crate opens FDs without CLOEXEC, which means they
persist across exec. This is not the convention throughout the Rust
ecosystem, which is to do the opposite:

rust-lang/rust#24237

This patch setting CLOEXEC :)

Note that doing that after-the-fact on the resulting FD isn't quite good
enough: if your process is forking a lot, that's racy.
Phantomical added a commit to Phantomical/perf-event that referenced this issue Apr 14, 2023
This appears to be what the rust stdlib does (see [0]) so following
along with their conventions is probably best.

This commit was inspired by this pr to perf-event:
jimblandy/perf-event#29

[0]: rust-lang/rust#24237
Phantomical added a commit to Phantomical/perf-event that referenced this issue Apr 20, 2023
This appears to be what the rust stdlib does (see [0]) so following
along with their conventions is probably best.

This commit was inspired by this pr to perf-event:
jimblandy/perf-event#29

[0]: rust-lang/rust#24237
Phantomical added a commit to Phantomical/perf-event that referenced this issue Apr 20, 2023
This appears to be what the rust stdlib does (see [0]) so following
along with their conventions is probably best.

This commit was inspired by this pr to perf-event:
jimblandy/perf-event#29

[0]: rust-lang/rust#24237
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants