Auto merge of #24034 - alexcrichton:cloexec, r=aturon

The commit messages have more details as to what's going on, but this is a breaking change for any libraries which expect file descriptors to be inherited by default.

Closes #12148

2 files changed, 43 insertions, 22 deletions

diff --git a/src/libstd/sys/windows/pipe2.rs b/src/libstd/sys/windows/pipe2.rs
index 229481e3d57..ed41c959782 100644
--- a/src/libstd/sys/windows/pipe2.rs
+++ b/src/libstd/sys/windows/pipe2.rs
@@ -22,22 +22,24 @@ 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];
-    match libc::pipe(fds.as_mut_ptr(), 1024 as ::libc::c_uint,
-    (libc::O_BINARY | libc::O_NOINHERIT) as c_int) {
-        0 => {
-            assert!(fds[0] != -1 && fds[0] != 0);
-            assert!(fds[1] != -1 && fds[1] != 0);
+    unsafe {
+        match libc::pipe(fds.as_mut_ptr(), 1024 as ::libc::c_uint,
+                         (libc::O_BINARY | libc::O_NOINHERIT) as c_int) {
+            0 => {
+                assert!(fds[0] != -1 && fds[0] != 0);
+                assert!(fds[1] != -1 && fds[1] != 0);
 
-            Ok((AnonPipe::from_fd(fds[0]), AnonPipe::from_fd(fds[1])))
+                Ok((AnonPipe::from_fd(fds[0]), AnonPipe::from_fd(fds[1])))
+            }
+            _ => Err(io::Error::last_os_error()),
         }
-        _ => Err(io::Error::last_os_error()),
     }
 }
 
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);