std: Unconditionally close all file descriptors

The logic for only closing file descriptors >= 3 was inherited from quite some
time ago and ends up meaning that some internal APIs are less consistent than
they should be. By unconditionally closing everything entering a `FileDesc` we
ensure that we're consistent in our behavior as well as robustly handling the
stdio case.

5 files changed, 86 insertions, 67 deletions

diff --git a/src/libstd/process.rs b/src/libstd/process.rs
index 9929593ffcb..90eabaee77b 100644
--- a/src/libstd/process.rs
+++ b/src/libstd/process.rs
@@ -19,13 +19,13 @@ use io::prelude::*;
 use ffi::OsStr;
 use fmt;
 use io::{self, Error, ErrorKind};
-use libc;
 use path;
 use sync::mpsc::{channel, Receiver};
 use sys::pipe2::{self, AnonPipe};
 use sys::process2::Command as CommandImp;
 use sys::process2::Process as ProcessImp;
 use sys::process2::ExitStatus as ExitStatusImp;
+use sys::process2::Stdio as StdioImp2;
 use sys_common::{AsInner, AsInnerMut};
 use thread;
 
@@ -229,13 +229,13 @@ impl Command {
 
     fn spawn_inner(&self, default_io: StdioImp) -> io::Result<Child> {
         let (their_stdin, our_stdin) = try!(
-            setup_io(self.stdin.as_ref().unwrap_or(&default_io), 0, true)
+            setup_io(self.stdin.as_ref().unwrap_or(&default_io), true)
         );
         let (their_stdout, our_stdout) = try!(
-            setup_io(self.stdout.as_ref().unwrap_or(&default_io), 1, false)
+            setup_io(self.stdout.as_ref().unwrap_or(&default_io), false)
         );
         let (their_stderr, our_stderr) = try!(
-            setup_io(self.stderr.as_ref().unwrap_or(&default_io), 2, false)
+            setup_io(self.stderr.as_ref().unwrap_or(&default_io), false)
         );
 
         match ProcessImp::spawn(&self.inner, their_stdin, their_stdout, their_stderr) {
@@ -328,23 +328,19 @@ impl AsInnerMut<CommandImp> for Command {
     fn as_inner_mut(&mut self) -> &mut CommandImp { &mut self.inner }
 }
 
-fn setup_io(io: &StdioImp, fd: libc::c_int, readable: bool)
-            -> io::Result<(Option<AnonPipe>, Option<AnonPipe>)>
+fn setup_io(io: &StdioImp, readable: bool)
+            -> io::Result<(StdioImp2, Option<AnonPipe>)>
 {
     use self::StdioImp::*;
     Ok(match *io {
-        Null => {
-            (None, None)
-        }
-        Inherit => {
-            (Some(AnonPipe::from_fd(fd)), None)
-        }
+        Null => (StdioImp2::None, None),
+        Inherit => (StdioImp2::Inherit, None),
         Piped => {
             let (reader, writer) = try!(pipe2::anon_pipe());
             if readable {
-                (Some(reader), Some(writer))
+                (StdioImp2::Piped(reader), Some(writer))
             } else {
-                (Some(writer), Some(reader))
+                (StdioImp2::Piped(writer), Some(reader))
             }
         }
     })
diff --git a/src/libstd/sys/unix/fd.rs b/src/libstd/sys/unix/fd.rs
index 4ef09b91c25..d86c77624e8 100644
--- a/src/libstd/sys/unix/fd.rs
+++ b/src/libstd/sys/unix/fd.rs
@@ -59,13 +59,6 @@ impl FileDesc {
             debug_assert_eq!(ret, 0);
         }
     }
-
-    pub fn unset_cloexec(&self) {
-        unsafe {
-            let ret = c::ioctl(self.fd, c::FIONCLEX);
-            debug_assert_eq!(ret, 0);
-        }
-    }
 }
 
 impl AsInner<c_int> for FileDesc {
@@ -74,14 +67,11 @@ impl AsInner<c_int> for FileDesc {
 
 impl Drop for FileDesc {
     fn drop(&mut self) {
-        // closing stdio file handles makes no sense, so never do it. Also, note
-        // that errors are ignored when closing a file descriptor. The reason
-        // for this is that if an error occurs we don't actually know if the
-        // file descriptor was closed or not, and if we retried (for something
-        // like EINTR), we might close another valid file descriptor (opened
-        // after we closed ours.
-        if self.fd > libc::STDERR_FILENO {
-            let _ = unsafe { libc::close(self.fd) };
-        }
+        // Note that errors are ignored when closing a file descriptor. The
+        // reason for this is that if an error occurs we don't actually know if
+        // the file descriptor was closed or not, and if we retried (for
+        // something like EINTR), we might close another valid file descriptor
+        // (opened after we closed ours.
+        let _ = unsafe { libc::close(self.fd) };
     }
 }
diff --git a/src/libstd/sys/unix/process2.rs b/src/libstd/sys/unix/process2.rs
index dc6897fec8e..0b4e871454d 100644
--- a/src/libstd/sys/unix/process2.rs
+++ b/src/libstd/sys/unix/process2.rs
@@ -119,6 +119,12 @@ pub struct Process {
     pid: pid_t
 }
 
+pub enum Stdio {
+    Inherit,
+    Piped(AnonPipe),
+    None,
+}
+
 const CLOEXEC_MSG_FOOTER: &'static [u8] = b"NOEX";
 
 impl Process {
@@ -128,9 +134,9 @@ impl Process {
     }
 
     pub fn spawn(cfg: &Command,
-                 in_fd: Option<AnonPipe>,
-                 out_fd: Option<AnonPipe>,
-                 err_fd: Option<AnonPipe>) -> io::Result<Process> {
+                 in_fd: Stdio,
+                 out_fd: Stdio,
+                 err_fd: Stdio) -> io::Result<Process> {
         let dirp = cfg.cwd.as_ref().map(|c| c.as_ptr()).unwrap_or(ptr::null());
 
         let (envp, _a, _b) = make_envp(cfg.env.as_ref());
@@ -224,9 +230,9 @@ impl Process {
                                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>) -> ! {
+                               in_fd: Stdio,
+                               out_fd: Stdio,
+                               err_fd: Stdio) -> ! {
         fn fail(output: &mut AnonPipe) -> ! {
             let errno = sys::os::errno() as u32;
             let bytes = [
@@ -244,23 +250,30 @@ impl Process {
             unsafe { libc::_exit(1) }
         }
 
-        // 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| {
-            src.map(|p| p.into_fd()).or_else(|| {
-                let mut opts = OpenOptions::new();
-                opts.read(dst == libc::STDIN_FILENO);
-                opts.write(dst != libc::STDIN_FILENO);
-                let devnull = CStr::from_ptr(b"/dev/null\0".as_ptr()
-                                                as *const _);
-                File::open_c(devnull, &opts).ok().map(|f| f.into_fd())
-            }).map(|fd| {
-                fd.unset_cloexec();
-                retry(|| libc::dup2(fd.raw(), dst)) != -1
-            }).unwrap_or(false)
+        let setup = |src: Stdio, dst: c_int| {
+            let fd = match src {
+                Stdio::Inherit => return true,
+                Stdio::Piped(pipe) => pipe.into_fd(),
+
+                // If a stdio file descriptor is set to be ignored, we 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.
+                Stdio::None => {
+                    let mut opts = OpenOptions::new();
+                    opts.read(dst == libc::STDIN_FILENO);
+                    opts.write(dst != libc::STDIN_FILENO);
+                    let devnull = CStr::from_ptr(b"/dev/null\0".as_ptr()
+                                                    as *const _);
+                    if let Ok(f) = File::open_c(devnull, &opts) {
+                        f.into_fd()
+                    } else {
+                        return false
+                    }
+                }
+            };
+            retry(|| libc::dup2(fd.raw(), dst)) != -1
         };
 
         if !setup(in_fd, libc::STDIN_FILENO) { fail(&mut output) }
diff --git a/src/libstd/sys/windows/process2.rs b/src/libstd/sys/windows/process2.rs
index 7e832b6384d..74953921921 100644
--- a/src/libstd/sys/windows/process2.rs
+++ b/src/libstd/sys/windows/process2.rs
@@ -105,11 +105,18 @@ pub struct Process {
     handle: Handle,
 }
 
+pub enum Stdio {
+    Inherit,
+    Piped(AnonPipe),
+    None,
+}
+
 impl Process {
     #[allow(deprecated)]
     pub fn spawn(cfg: &Command,
-                 in_fd: Option<AnonPipe>, out_fd: Option<AnonPipe>, err_fd: Option<AnonPipe>)
-                 -> io::Result<Process>
+                 in_fd: Stdio,
+                 out_fd: Stdio,
+                 err_fd: Stdio) -> io::Result<Process>
     {
         use libc::types::os::arch::extra::{DWORD, HANDLE, STARTUPINFO};
         use libc::consts::os::extra::{
@@ -156,13 +163,16 @@ impl Process {
 
             let cur_proc = GetCurrentProcess();
 
-            // Similarly to unix, we don't actually leave holes for the stdio file
-            // descriptors, but rather open up /dev/null equivalents. These
-            // equivalents are drawn from libuv's windows process spawning.
-            let set_fd = |fd: &Option<AnonPipe>, slot: &mut HANDLE,
+            let set_fd = |fd: &Stdio, slot: &mut HANDLE,
                           is_stdin: bool| {
                 match *fd {
-                    None => {
+                    Stdio::Inherit => {}
+
+                    // Similarly to unix, we don't actually leave holes for the
+                    // stdio file descriptors, but rather open up /dev/null
+                    // equivalents. These equivalents are drawn from libuv's
+                    // windows process spawning.
+                    Stdio::None => {
                         let access = if is_stdin {
                             libc::FILE_GENERIC_READ
                         } else {
@@ -188,11 +198,8 @@ impl Process {
                             return Err(Error::last_os_error())
                         }
                     }
-                    Some(ref pipe) => {
+                    Stdio::Piped(ref pipe) => {
                         let orig = pipe.raw();
-                        if orig == INVALID_HANDLE_VALUE {
-                            return Err(Error::last_os_error())
-                        }
                         if DuplicateHandle(cur_proc, orig, cur_proc, slot,
                                            0, TRUE, DUPLICATE_SAME_ACCESS) == FALSE {
                             return Err(Error::last_os_error())
@@ -235,9 +242,15 @@ impl Process {
                 })
             });
 
-            assert!(CloseHandle(si.hStdInput) != 0);
-            assert!(CloseHandle(si.hStdOutput) != 0);
-            assert!(CloseHandle(si.hStdError) != 0);
+            if !in_fd.inherited() {
+                assert!(CloseHandle(si.hStdInput) != 0);
+            }
+            if !out_fd.inherited() {
+                assert!(CloseHandle(si.hStdOutput) != 0);
+            }
+            if !err_fd.inherited() {
+                assert!(CloseHandle(si.hStdError) != 0);
+            }
 
             match create_err {
                 Some(err) => return Err(err),
@@ -296,6 +309,12 @@ impl Process {
     }
 }
 
+impl Stdio {
+    fn inherited(&self) -> bool {
+        match *self { Stdio::Inherit => true, _ => false }
+    }
+}
+
 #[derive(PartialEq, Eq, Clone, Copy, Debug)]
 pub struct ExitStatus(i32);
 
diff --git a/src/test/run-pass/fds-are-cloexec.rs b/src/test/run-pass/fds-are-cloexec.rs
index ec59b4dc03d..cbf7830513a 100644
--- a/src/test/run-pass/fds-are-cloexec.rs
+++ b/src/test/run-pass/fds-are-cloexec.rs
@@ -9,6 +9,7 @@
 // except according to those terms.
 
 // ignore-windows
+// ignore-android
 
 #![feature(libc)]