std: Clean up process spawn impl on unix

* De-indent quite a bit by removing usage of FnOnce closures
* Clearly separate code for the parent/child after the fork
* Use `fs2::{File, OpenOptions}` instead of calling `open` manually
* Use RAII to close I/O objects wherever possible
* Remove loop for closing all file descriptors, all our own ones are now
  `CLOEXEC` by default so they cannot be inherited

src/libstd/process.rs

@@ -340,7 +340,7 @@ fn setup_io(io: &StdioImp, fd: libc::c_int, readable: bool)
             (Some(AnonPipe::from_fd(fd)), None)
         }
         Piped => {
-            let (reader, writer) = try!(unsafe { pipe2::anon_pipe() });
+            let (reader, writer) = try!(pipe2::anon_pipe());
             if readable {
                 (Some(reader), Some(writer))
             } else {

src/libstd/sys/unix/c.rs

@@ -159,6 +159,8 @@ extern {
     pub fn utimes(filename: *const libc::c_char,
                   times: *const libc::timeval) -> libc::c_int;
     pub fn gai_strerror(errcode: libc::c_int) -> *const libc::c_char;
+    pub fn setgroups(ngroups: libc::c_int,
+                     ptr: *const libc::c_void) -> libc::c_int;
 }
 
 #[cfg(any(target_os = "macos", target_os = "ios"))]

src/libstd/sys/unix/fs2.rs

@@ -205,13 +205,17 @@ impl OpenOptions {
 
 impl File {
     pub fn open(path: &Path, opts: &OpenOptions) -> io::Result<File> {
+        let path = try!(cstr(path));
+        File::open_c(&path, opts)
+    }
+
+    pub fn open_c(path: &CStr, opts: &OpenOptions) -> io::Result<File> {
         let flags = opts.flags | match (opts.read, opts.write) {
             (true, true) => libc::O_RDWR,
             (false, true) => libc::O_WRONLY,
             (true, false) |
             (false, false) => libc::O_RDONLY,
         };
-        let path = try!(cstr(path));
         let fd = try!(cvt_r(|| unsafe {
             libc::open(path.as_ptr(), flags, opts.mode)
         }));
@@ -220,6 +224,8 @@ impl File {
         Ok(File(fd))
     }
 
+    pub fn into_fd(self) -> FileDesc { self.0 }
+
     pub fn file_attr(&self) -> io::Result<FileAttr> {
         let mut stat: libc::stat = unsafe { mem::zeroed() };
         try!(cvt(unsafe { libc::fstat(self.0.raw(), &mut stat) }));

src/libstd/sys/unix/pipe2.rs

@@ -20,11 +20,10 @@ use libc;
 
 pub struct AnonPipe(FileDesc);
 
-pub unsafe fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
+pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
     let mut fds = [0; 2];
-    if libc::pipe(fds.as_mut_ptr()) == 0 {
+    if unsafe { libc::pipe(fds.as_mut_ptr()) == 0 } {
-        Ok((AnonPipe::from_fd(fds[0]),
+        Ok((AnonPipe::from_fd(fds[0]), AnonPipe::from_fd(fds[1])))
-            AnonPipe::from_fd(fds[1])))
     } else {
         Err(io::Error::last_os_error())
     }
@@ -45,7 +44,7 @@ impl AnonPipe {
         self.0.write(buf)
     }
 
-    pub fn raw(&self) -> libc::c_int {
+    pub fn into_fd(self) -> FileDesc {
-        self.0.raw()
+        self.0
     }
 }

src/libstd/sys/unix/process2.rs

@@ -13,14 +13,14 @@ use os::unix::prelude::*;
 
 use collections::HashMap;
 use env;
-use ffi::{OsString, OsStr, CString};
+use ffi::{OsString, OsStr, CString, CStr};
 use fmt;
 use io::{self, Error, ErrorKind};
 use libc::{self, pid_t, c_void, c_int, gid_t, uid_t};
-use mem;
 use ptr;
 use sys::pipe2::AnonPipe;
 use sys::{self, retry, c, cvt};
+use sys::fs2::{File, OpenOptions};
 
 ////////////////////////////////////////////////////////////////////////////////
 // Command
@@ -128,65 +128,35 @@ impl Process {
     }
 
     pub fn spawn(cfg: &Command,
-                 in_fd: Option<AnonPipe>, out_fd: Option<AnonPipe>, err_fd: Option<AnonPipe>)
+                 in_fd: Option<AnonPipe>,
-                 -> io::Result<Process>
+                 out_fd: Option<AnonPipe>,
-    {
+                 err_fd: Option<AnonPipe>) -> io::Result<Process> {
-        use libc::funcs::posix88::unistd::{fork, dup2, close, chdir, execvp};
-
-        mod rustrt {
-            extern {
-                pub fn rust_unset_sigprocmask();
-            }
-        }
-
-        unsafe fn set_cloexec(fd: c_int) {
-            let ret = c::ioctl(fd, c::FIOCLEX);
-            assert_eq!(ret, 0);
-        }
-
-        #[cfg(all(target_os = "android", target_arch = "aarch64"))]
-        unsafe fn getdtablesize() -> c_int {
-            libc::sysconf(libc::consts::os::sysconf::_SC_OPEN_MAX) as c_int
-        }
-
-        #[cfg(not(all(target_os = "android", target_arch = "aarch64")))]
-        unsafe fn getdtablesize() -> c_int {
-            libc::funcs::bsd44::getdtablesize()
-        }
-
         let dirp = cfg.cwd.as_ref().map(|c| c.as_ptr()).unwrap_or(ptr::null());
 
-        with_envp(cfg.env.as_ref(), |envp: *const c_void| {
+        let (envp, _a, _b) = make_envp(cfg.env.as_ref());
-            with_argv(&cfg.program, &cfg.args, |argv: *const *const libc::c_char| unsafe {
+        let (argv, _a) = make_argv(&cfg.program, &cfg.args);
-                let (input, mut output) = try!(sys::pipe2::anon_pipe());
+        let (input, output) = try!(sys::pipe2::anon_pipe());
 
-                // We may use this in the child, so perform allocations before the
+        let pid = unsafe {
-                // fork
+            match libc::fork() {
-                let devnull = b"/dev/null\0";
+                0 => {
-
+                    drop(input);
-                set_cloexec(output.raw());
+                    Process::child_after_fork(cfg, output, argv, envp, dirp,
-
+                                              in_fd, out_fd, err_fd)
-                let pid = fork();
-                if pid < 0 {
-                    return Err(Error::last_os_error())
-                } else if pid > 0 {
-                    #[inline]
-                    fn combine(arr: &[u8]) -> i32 {
-                        let a = arr[0] as u32;
-                        let b = arr[1] as u32;
-                        let c = arr[2] as u32;
-                        let d = arr[3] as u32;
-
-                        ((a << 24) | (b << 16) | (c << 8) | (d << 0)) as i32
                 }
+                n if n < 0 => return Err(Error::last_os_error()),
+                n => n,
+            }
+        };
 
         let p = Process{ pid: pid };
         drop(output);
         let mut bytes = [0; 8];
 
-                    // loop to handle EINTER
+        // loop to handle EINTR
         loop {
             match input.read(&mut bytes) {
+                Ok(0) => return Ok(p),
                 Ok(8) => {
                     assert!(combine(CLOEXEC_MSG_FOOTER) == combine(&bytes[4.. 8]),
                             "Validation on the CLOEXEC pipe failed: {:?}", bytes);
@@ -195,7 +165,6 @@ impl Process {
                             "wait() should either return Ok or panic");
                     return Err(Error::from_raw_os_error(errno))
                 }
-                            Ok(0) => return Ok(p),
                 Err(ref e) if e.kind() == ErrorKind::Interrupted => {}
                 Err(e) => {
                     assert!(p.wait().is_ok(),
@@ -209,6 +178,15 @@ impl Process {
                 }
             }
         }
+
+        fn combine(arr: &[u8]) -> i32 {
+            let a = arr[0] as u32;
+            let b = arr[1] as u32;
+            let c = arr[2] as u32;
+            let d = arr[3] as u32;
+
+            ((a << 24) | (b << 16) | (c << 8) | (d << 0)) as i32
+        }
     }
 
     // And at this point we've reached a special time in the life of the
@@ -241,8 +219,14 @@ impl Process {
     // allocation). Instead we just close it manually. This will never
     // have the drop glue anyway because this code never returns (the
     // child will either exec() or invoke libc::exit)
-                let _ = libc::close(input.raw());
+    unsafe fn child_after_fork(cfg: &Command,
-
+                               mut output: AnonPipe,
+                               argv: *const *const libc::c_char,
+                               envp: *const libc::c_void,
+                               dirp: *const libc::c_char,
+                               in_fd: Option<AnonPipe>,
+                               out_fd: Option<AnonPipe>,
+                               err_fd: Option<AnonPipe>) -> ! {
         fn fail(output: &mut AnonPipe) -> ! {
             let errno = sys::os::errno() as u32;
             let bytes = [
@@ -253,61 +237,42 @@ impl Process {
                 CLOEXEC_MSG_FOOTER[0], CLOEXEC_MSG_FOOTER[1],
                 CLOEXEC_MSG_FOOTER[2], CLOEXEC_MSG_FOOTER[3]
             ];
-                    // pipe I/O up to PIPE_BUF bytes should be atomic
+            // pipe I/O up to PIPE_BUF bytes should be atomic, and then we want
+            // to be sure we *don't* run at_exit destructors as we're being torn
+            // down regardless
             assert!(output.write(&bytes).is_ok());
             unsafe { libc::_exit(1) }
         }
 
-                rustrt::rust_unset_sigprocmask();
-
         // If a stdio file descriptor is set to be ignored, we don't
         // actually close it, but rather open up /dev/null into that
         // file descriptor. Otherwise, the first file descriptor opened
         // up in the child would be numbered as one of the stdio file
         // descriptors, which is likely to wreak havoc.
         let setup = |src: Option<AnonPipe>, dst: c_int| {
-                    let src = match src {
+            src.map(|p| p.into_fd()).or_else(|| {
-                        None => {
+                let mut opts = OpenOptions::new();
-                            let flags = if dst == libc::STDIN_FILENO {
+                opts.read(dst == libc::STDIN_FILENO);
-                                libc::O_RDONLY
+                opts.write(dst != libc::STDIN_FILENO);
-                            } else {
+                let devnull = CStr::from_ptr(b"/dev/null\0".as_ptr()
-                                libc::O_RDWR
+                                                as *const _);
-                            };
+                File::open_c(devnull, &opts).ok().map(|f| f.into_fd())
-                            libc::open(devnull.as_ptr() as *const _, flags, 0)
+            }).map(|fd| {
-                        }
+                fd.unset_cloexec();
-                        Some(obj) => {
+                retry(|| libc::dup2(fd.raw(), dst)) != -1
-                            let fd = obj.raw();
+            }).unwrap_or(false)
-                            // Leak the memory and the file descriptor. We're in the
-                            // child now an all our resources are going to be
-                            // cleaned up very soon
-                            mem::forget(obj);
-                            fd
-                        }
-                    };
-                    src != -1 && retry(|| dup2(src, dst)) != -1
         };
 
         if !setup(in_fd, libc::STDIN_FILENO) { fail(&mut output) }
         if !setup(out_fd, libc::STDOUT_FILENO) { fail(&mut output) }
         if !setup(err_fd, libc::STDERR_FILENO) { fail(&mut output) }
 
-                // close all other fds
+        if let Some(u) = cfg.gid {
-                for fd in (3..getdtablesize()).rev() {
-                    if fd != output.raw() {
-                        let _ = close(fd as c_int);
-                    }
-                }
-
-                match cfg.gid {
-                    Some(u) => {
             if libc::setgid(u as libc::gid_t) != 0 {
                 fail(&mut output);
             }
         }
-                    None => {}
+        if let Some(u) = cfg.uid {
-                }
-                match cfg.uid {
-                    Some(u) => {
             // When dropping privileges from root, the `setgroups` call
             // will remove any extraneous groups. If we don't call this,
             // then even though our uid has dropped, we may still have
@@ -315,34 +280,26 @@ impl Process {
             // fail if we aren't root, so don't bother checking the
             // return value, this is just done as an optimistic
             // privilege dropping function.
-                        extern {
+            let _ = c::setgroups(0, ptr::null());
-                            fn setgroups(ngroups: libc::c_int,
-                                         ptr: *const libc::c_void) -> libc::c_int;
-                        }
-                        let _ = setgroups(0, ptr::null());
 
             if libc::setuid(u as libc::uid_t) != 0 {
                 fail(&mut output);
             }
         }
-                    None => {}
-                }
         if cfg.detach {
             // Don't check the error of setsid because it fails if we're the
             // process leader already. We just forked so it shouldn't return
             // error, but ignore it anyway.
             let _ = libc::setsid();
         }
-                if !dirp.is_null() && chdir(dirp) == -1 {
+        if !dirp.is_null() && libc::chdir(dirp) == -1 {
             fail(&mut output);
         }
         if !envp.is_null() {
             *sys::os::environ() = envp as *const _;
         }
-                let _ = execvp(*argv, argv as *mut _);
+        let _ = libc::execvp(*argv, argv as *mut _);
-                fail(&mut output);
+        fail(&mut output)
-            })
-        })
     }
 
     pub fn wait(&self) -> io::Result<ExitStatus> {
@@ -364,8 +321,8 @@ impl Process {
     }
 }
 
-fn with_argv<T,F>(prog: &CString, args: &[CString], cb: F) -> T
+fn make_argv(prog: &CString, args: &[CString])
-    where F : FnOnce(*const *const libc::c_char) -> T
+             -> (*const *const libc::c_char, Vec<*const libc::c_char>)
 {
     let mut ptrs: Vec<*const libc::c_char> = Vec::with_capacity(args.len()+1);
 
@@ -380,19 +337,18 @@ fn with_argv<T,F>(prog: &CString, args: &[CString], cb: F) -> T
     // Add a terminating null pointer (required by libc).
     ptrs.push(ptr::null());
 
-    cb(ptrs.as_ptr())
+    (ptrs.as_ptr(), ptrs)
 }
 
-fn with_envp<T, F>(env: Option<&HashMap<OsString, OsString>>, cb: F) -> T
+fn make_envp(env: Option<&HashMap<OsString, OsString>>)
-    where F : FnOnce(*const c_void) -> T
+             -> (*const c_void, Vec<Vec<u8>>, Vec<*const libc::c_char>)
 {
     // On posixy systems we can pass a char** for envp, which is a
     // null-terminated array of "k=v\0" strings. Since we must create
     // these strings locally, yet expose a raw pointer to them, we
     // create a temporary vector to own the CStrings that outlives the
     // call to cb.
-    match env {
+    if let Some(env) = env {
-        Some(env) => {
         let mut tmps = Vec::with_capacity(env.len());
 
         for pair in env {
@@ -404,16 +360,15 @@ fn with_envp<T, F>(env: Option<&HashMap<OsString, OsString>>, cb: F) -> T
             tmps.push(kv);
         }
 
-            // As with `with_argv`, this is unsafe, since cb could leak the pointers.
         let mut ptrs: Vec<*const libc::c_char> =
             tmps.iter()
                 .map(|tmp| tmp.as_ptr() as *const libc::c_char)
                 .collect();
         ptrs.push(ptr::null());
 
-            cb(ptrs.as_ptr() as *const c_void)
+        (ptrs.as_ptr() as *const _, tmps, ptrs)
-        }
+    } else {
-        _ => cb(ptr::null())
+        (0 as *const _, Vec::new(), Vec::new())
     }
 }
 

src/libstd/sys/windows/pipe2.rs

@@ -22,13 +22,14 @@ pub struct AnonPipe {
     fd: c_int
 }
 
-pub unsafe fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
+pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
     // Windows pipes work subtly differently than unix pipes, and their
     // inheritance has to be handled in a different way that I do not
     // fully understand. Here we explicitly make the pipe non-inheritable,
     // which means to pass it to a subprocess they need to be duplicated
     // first, as in std::run.
     let mut fds = [0; 2];
+    unsafe {
         match libc::pipe(fds.as_mut_ptr(), 1024 as ::libc::c_uint,
                          (libc::O_BINARY | libc::O_NOINHERIT) as c_int) {
             0 => {
@@ -40,6 +41,7 @@ pub unsafe fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
             _ => Err(io::Error::last_os_error()),
         }
     }
+}
 
 impl AnonPipe {
     pub fn from_fd(fd: libc::c_int) -> AnonPipe {