Atomically set CLOEXEC on duplicated sockets #27980

tbu- · 2015-08-24T19:40:22Z

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

rust-highfive · 2015-08-24T19:40:38Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

tbu- · 2015-08-24T19:41:19Z

@barosl Could you provide the constants for the different OSs again, please?

tbu- · 2015-08-24T19:45:17Z

r? @alexcrichton

nagisa · 2015-08-24T20:06:16Z

src/liblibc/lib.rs

+        #[cfg(target_os = "freebsd")]
+        pub mod posix08 {
+            use types::os::arch::c95::c_int;
+            pub const O_CLOEXEC: c_int = 0x100000;


pub const F_DUPFD_CLOEXEC : c_int = 17;

tbu- · 2015-08-24T20:20:01Z

@nagisa May I ask how you get all these constants? Do you have virtual machines? Do you look them up in the source code?

nagisa · 2015-08-24T20:22:46Z

I looked them up in the source code using crossreferences available online. e.g. FreeBSD or XNU/OS X

nagisa · 2015-08-24T20:23:49Z

Is it necessary to base this PR onto #27971 instead of master?

tbu- · 2015-08-24T20:25:13Z

@nagisa Yes, otherwise merge conflicts would occur around the posix08 constants.

alexcrichton · 2015-08-24T21:11:21Z

Can this also avoid copying the constant definition in liblibc? This probably is not the only mistake in the organization of liblibc, so it's better to reduce the duplication than try to keep it correct from where it is today.

tbu- · 2015-08-24T21:12:22Z

@alexcrichton Should the new constants be put in the incorrect module as well (for consistency) or in the posix08 module (for correctness)?

alexcrichton · 2015-08-24T21:12:29Z

src/libstd/sys/unix/net.rs

@@ -66,7 +67,30 @@ impl Socket {
    }

    pub fn duplicate(&self) -> io::Result<Socket> {
-        let fd = try!(cvt(unsafe { libc::dup(self.0.raw()) }));
+        use libc::funcs::posix88::fcntl::fcntl;
+        const ZERO: c_int = 0;


How come this and MINUS_ONE are constants? Could they just be literals and/or locals?

Not sure why I added MINUS_ONE, and will remove that, but ZERO is kind of required to make sure the type of 0 is c_int as this is passed as varargs to fcntl.

Does anything go wrong if it's just passed as a function argument?

I can't guarantee it's c_int then, because the compiler doesn't (/can't) know the type of the third parameter. Whether anything goes wrong or not, one would need to have better understanding of this situation than I do because this is unsafe code.

(For reference, the function signature is declared as int fcntl(int filedes, int cmd, ...), and in our case int fcntl(int filedes, F_DUPFD_CLOEXEC, int arg).)

alexcrichton · 2015-08-24T21:13:36Z

So long as they find their way into either liblibc or sys::c I don't particularly mind where they're located.

alexcrichton · 2015-08-24T21:16:27Z

src/libstd/sys/unix/net.rs

+                }
+            });
+            unsafe { FCNTL_DUPFD }
+        }


Can you add a comment explaining what this function is doing? I'm also somewhat wary about the assert! here as there's probably some random kernel which returns some obscure error code. Are we also guaranteed that EINVAL is never returned for a bad file descriptor?

I'm not sure if there's some prior art to draw from in this sort of detection, but I'm a little wary of this...

barosl · 2015-08-24T21:25:17Z

Here you are!

Linux: 1030 (0x406)
FreeBSD: 17 (0x11)
OpenBSD: 10 (0xa)
NetBSD: 12 (0xc)
Dragonfly: 17 (0x11)
Bitrig: 10 (0xa)

NetBSD's seems to differ from the current value of 10.

Tested on my VMs.

tbu- · 2015-08-24T23:09:38Z

@alexcrichton Added a comment and referenced prior art: http://comments.gmane.org/gmane.linux.lib.musl.general/2963.

barosl · 2015-08-25T04:40:06Z

src/liblibc/lib.rs

@@ -4632,7 +4646,7 @@ pub mod consts {
            pub const F_GETLK : c_int = 7;
            pub const F_SETLK : c_int = 8;
            pub const F_SETLKW : c_int = 9;
-            pub const F_DUPFD_CLOEXEC : c_int = 10;
+            pub use consts::os::posix08::F_DUPFD_CLOEXEC;


Actually, #27930 already introduced a breaking change to libc, which changed the location of _PC_NAME_MAX from the funcs module (!) to the consts module. So personally, I think it would be clearer if this got removed completely, as we're committing breakage anyways. But YMMV!

alexcrichton · 2015-08-25T06:15:33Z

Hm it looks like the implementation here is pretty different from that inside MUSL. The mailing list thread indicates that there are two conditions we can get EINVAL:

The file descriptor argument is too big
The cloexec variant isn't supported

It's probably tough to say where EBADF vs EINVAL is returned, and that thread doesn't seem to have a conclusion about whether EINVAL could perhaps be returned for bad file descriptors. How about switching up the implementation to the just-as-efficient (on newer systems at least) but simpler (and closer to what MUSL does):

pub fn duplicate(&self) -> io::Result<Socket> {
    cvt(unsafe {
        libc::fcntl(self.0.raw(), libc::FCNTL_DUPFD_CLOEXEC, 0)
    }).or_else(|e| {
        if e.raw_os_error() == Some(libc::EINVAL) {
            cvt(unsafe { libc::fcntl(self.0.raw(), libc::FCNTL_DUPFD, 0) })
        } else {
            Err(e)
        }
    }).map(|fd| {
        let fd = FileDesc::new();
        fd.set_cloexec();
        Socket(fd)
    })
}

We know that the first case of EINVAL can't be returned because we have a valid file descriptor so that's all we need to check for. It's also not clear to me whether this is worth the current platform-specific logic plus the caching via Once, so slimming it down and unifying it for all platforms is a bonus.

tbu- · 2015-08-25T07:16:56Z

@alexcrichton The documentation of the syscall and the linked mailing list thread agree on that EINVAL is only returned on unknown command or incorrect third parameter, but we know that 0 is always valid here, so it must be the bad command. The kernel source code first switches on the command (and it has to, because the first parameter can have different meanings depending on the command), so EBADF is returned later than EINVAL on invalid first parameter.

I'll try to get closer to MUSL though...

tbu- · 2015-08-25T07:50:52Z

@alexcrichton It's now nearer to the MUSL code.

alexcrichton · 2015-08-25T16:38:47Z

src/libstd/sys/unix/net.rs

+                Err(ref e) if e.raw_os_error() == Some(libc::EINVAL) => {
+                    EMULATE_F_DUPFD_CLOEXEC.store(true, atomic::Ordering::Relaxed);
+                }
+                res => return res.map(|fd| Socket(FileDesc::new(fd))),


An interesting point of the MUSL thread indicated that some newer kernels apparently broke F_DUPFD_CLOEXEC such that it was the same as F_DUPFD, and hence even if this syscall succeeds they still set cloexec. It may be good to just keep the same common code at the end where set_cloexec is called?

tbu- · 2015-08-29T22:38:26Z

@alexcrichton Done.

alexcrichton · 2015-08-29T23:38:13Z

Looks good to me, thanks! r=me with a squash and a rebase

For Bitrig, NetBSD and OpenBSD the constant was incorrectly in posix01, when it's actually posix08, so we move it. This is a [breaking-change], but we already had one in rust-lang#27930. Fix NetBSD's F_DUPFD_CLOEXEC constant. For a similar feature detection, see this musl thread: http://comments.gmane.org/gmane.linux.lib.musl.general/2963 This assumes that an int literal has type `c_int` for varidic functions.

tbu- · 2015-08-30T08:41:00Z

@alexcrichton Rebased.

alexcrichton · 2015-08-31T06:47:37Z

@bors: r+ 1f81ef4

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 · 2015-08-31T07:24:40Z

⌛ Testing commit 1f81ef4 with merge 05cc464...

bors · 2015-08-31T09:15:53Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

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

rust-highfive assigned nikomatsakis Aug 24, 2015

rust-highfive assigned alexcrichton and unassigned nikomatsakis Aug 24, 2015

nagisa reviewed Aug 24, 2015
View reviewed changes

alexcrichton reviewed Aug 24, 2015
View reviewed changes

tbu- force-pushed the pr_cloexec_dup branch from e5d47a7 to edee43d Compare August 24, 2015 23:07

barosl reviewed Aug 25, 2015
View reviewed changes

alexcrichton reviewed Aug 25, 2015
View reviewed changes

tbu- force-pushed the pr_cloexec_dup branch from b3d6537 to 1f81ef4 Compare August 30, 2015 08:25

bors merged commit 1f81ef4 into rust-lang:master Aug 31, 2015

alexcrichton mentioned this pull request Oct 6, 2015

Leverage atomically setting CLOEXEC where possible #24237

Closed

5 tasks

alexcrichton mentioned this pull request Feb 4, 2016

Finish atomically setting CLOEXEC for fds created on Unix #31417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atomically set CLOEXEC on duplicated sockets #27980

Atomically set CLOEXEC on duplicated sockets #27980

tbu- commented Aug 24, 2015

rust-highfive commented Aug 24, 2015

tbu- commented Aug 24, 2015

tbu- commented Aug 24, 2015

nagisa Aug 24, 2015

tbu- commented Aug 24, 2015

nagisa commented Aug 24, 2015

nagisa commented Aug 24, 2015

tbu- commented Aug 24, 2015

alexcrichton commented Aug 24, 2015

tbu- commented Aug 24, 2015

alexcrichton Aug 24, 2015

tbu- Aug 24, 2015

alexcrichton Aug 24, 2015

tbu- Aug 24, 2015

alexcrichton commented Aug 24, 2015

alexcrichton Aug 24, 2015

barosl commented Aug 24, 2015

tbu- commented Aug 24, 2015

barosl Aug 25, 2015

alexcrichton commented Aug 25, 2015

tbu- commented Aug 25, 2015

tbu- commented Aug 25, 2015

alexcrichton Aug 25, 2015

tbu- commented Aug 29, 2015

alexcrichton commented Aug 29, 2015

tbu- commented Aug 30, 2015

alexcrichton commented Aug 31, 2015

bors commented Aug 31, 2015

bors commented Aug 31, 2015

Atomically set CLOEXEC on duplicated sockets #27980

Atomically set CLOEXEC on duplicated sockets #27980

Conversation

tbu- commented Aug 24, 2015

rust-highfive commented Aug 24, 2015

tbu- commented Aug 24, 2015

tbu- commented Aug 24, 2015

Choose a reason for hiding this comment

tbu- commented Aug 24, 2015

nagisa commented Aug 24, 2015

nagisa commented Aug 24, 2015

tbu- commented Aug 24, 2015

alexcrichton commented Aug 24, 2015

tbu- commented Aug 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Aug 24, 2015

Choose a reason for hiding this comment

barosl commented Aug 24, 2015

tbu- commented Aug 24, 2015

Choose a reason for hiding this comment

alexcrichton commented Aug 25, 2015

tbu- commented Aug 25, 2015

tbu- commented Aug 25, 2015

Choose a reason for hiding this comment

tbu- commented Aug 29, 2015

alexcrichton commented Aug 29, 2015

tbu- commented Aug 30, 2015

alexcrichton commented Aug 31, 2015

bors commented Aug 31, 2015

bors commented Aug 31, 2015