auto merge of #14477 : alexcrichton/rust/issue-14456, r=brson

When spawning a process, stdio file descriptors can be configured to be ignored,
which basically means that they'll be closed. Currently this is done by
literally closing the file descriptors in the child, but this can have adverse
side effects if the child process then opens a new file descriptor, assigning it
to a stdio number.

To work around the problems of the child, this commit alters the process
spawning code to map stdio fds to /dev/null on unix (and a similar equivalent on
windows) when they are specified as being ignored. This should allow spawned
programs to have more expected behavior when opening new files.

Closes #14456

Author: bors

src/liblibc/lib.rs

@@ -235,6 +235,7 @@ pub use funcs::bsd43::{shutdown};
 #[cfg(windows)] pub use types::os::arch::extra::{LARGE_INTEGER, LPVOID, LONG};
 #[cfg(windows)] pub use types::os::arch::extra::{time64_t, OVERLAPPED, LPCWSTR};
 #[cfg(windows)] pub use types::os::arch::extra::{LPOVERLAPPED, SIZE_T, LPDWORD};
+#[cfg(windows)] pub use types::os::arch::extra::{SECURITY_ATTRIBUTES};
 #[cfg(windows)] pub use funcs::c95::string::{wcslen};
 #[cfg(windows)] pub use funcs::posix88::stat_::{wstat, wutime, wchmod, wrmdir};
 #[cfg(windows)] pub use funcs::bsd43::{closesocket};
@@ -1140,8 +1141,12 @@ pub mod types {
                 pub type LPWCH = *mut WCHAR;
                 pub type LPCH = *mut CHAR;
 
-                // Not really, but opaque to us.
-                pub type LPSECURITY_ATTRIBUTES = LPVOID;
+                pub struct SECURITY_ATTRIBUTES {
+                    pub nLength: DWORD,
+                    pub lpSecurityDescriptor: LPVOID,
+                    pub bInheritHandle: BOOL,
+                }
+                pub type LPSECURITY_ATTRIBUTES = *mut SECURITY_ATTRIBUTES;
 
                 pub type LPVOID = *mut c_void;
                 pub type LPCVOID = *c_void;

src/libnative/io/process.rs

@@ -244,7 +244,8 @@ struct SpawnProcessResult {
 }
 
 #[cfg(windows)]
-fn spawn_process_os(cfg: ProcessConfig, in_fd: c_int, out_fd: c_int, err_fd: c_int)
+fn spawn_process_os(cfg: ProcessConfig,
+                    in_fd: c_int, out_fd: c_int, err_fd: c_int)
                  -> IoResult<SpawnProcessResult> {
     use libc::types::os::arch::extra::{DWORD, HANDLE, STARTUPINFO};
     use libc::consts::os::extra::{
@@ -278,38 +279,51 @@ fn spawn_process_os(cfg: ProcessConfig, in_fd: c_int, out_fd: c_int, err_fd: c_i
 
         let cur_proc = GetCurrentProcess();
 
-        if in_fd != -1 {
-            let orig_std_in = get_osfhandle(in_fd) as HANDLE;
-            if orig_std_in == INVALID_HANDLE_VALUE as HANDLE {
-                fail!("failure in get_osfhandle: {}", os::last_os_error());
-            }
-            if DuplicateHandle(cur_proc, orig_std_in, cur_proc, &mut si.hStdInput,
-                               0, TRUE, DUPLICATE_SAME_ACCESS) == FALSE {
-                fail!("failure in DuplicateHandle: {}", os::last_os_error());
-            }
-        }
-
-        if out_fd != -1 {
-            let orig_std_out = get_osfhandle(out_fd) as HANDLE;
-            if orig_std_out == INVALID_HANDLE_VALUE as HANDLE {
-                fail!("failure in get_osfhandle: {}", os::last_os_error());
-            }
-            if DuplicateHandle(cur_proc, orig_std_out, cur_proc, &mut si.hStdOutput,
-                               0, TRUE, DUPLICATE_SAME_ACCESS) == FALSE {
-                fail!("failure in DuplicateHandle: {}", os::last_os_error());
+        // 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: c_int, slot: &mut HANDLE, is_stdin: bool| {
+            if fd == -1 {
+                let access = if is_stdin {
+                    libc::FILE_GENERIC_READ
+                } else {
+                    libc::FILE_GENERIC_WRITE | libc::FILE_READ_ATTRIBUTES
+                };
+                let size = mem::size_of::<libc::SECURITY_ATTRIBUTES>();
+                let mut sa = libc::SECURITY_ATTRIBUTES {
+                    nLength: size as libc::DWORD,
+                    lpSecurityDescriptor: ptr::mut_null(),
+                    bInheritHandle: 1,
+                };
+                *slot = os::win32::as_utf16_p("NUL", |filename| {
+                    libc::CreateFileW(filename,
+                                      access,
+                                      libc::FILE_SHARE_READ |
+                                          libc::FILE_SHARE_WRITE,
+                                      &mut sa,
+                                      libc::OPEN_EXISTING,
+                                      0,
+                                      ptr::mut_null())
+                });
+                if *slot == INVALID_HANDLE_VALUE as libc::HANDLE {
+                    return Err(super::last_error())
+                }
+            } else {
+                let orig = get_osfhandle(fd) as HANDLE;
+                if orig == INVALID_HANDLE_VALUE as HANDLE {
+                    return Err(super::last_error())
+                }
+                if DuplicateHandle(cur_proc, orig, cur_proc, slot,
+                                   0, TRUE, DUPLICATE_SAME_ACCESS) == FALSE {
+                    return Err(super::last_error())
+                }
             }
-        }
+            Ok(())
+        };
 
-        if err_fd != -1 {
-            let orig_std_err = get_osfhandle(err_fd) as HANDLE;
-            if orig_std_err == INVALID_HANDLE_VALUE as HANDLE {
-                fail!("failure in get_osfhandle: {}", os::last_os_error());
-            }
-            if DuplicateHandle(cur_proc, orig_std_err, cur_proc, &mut si.hStdError,
-                               0, TRUE, DUPLICATE_SAME_ACCESS) == FALSE {
-                fail!("failure in DuplicateHandle: {}", os::last_os_error());
-            }
-        }
+        try!(set_fd(in_fd, &mut si.hStdInput, true));
+        try!(set_fd(out_fd, &mut si.hStdOutput, false));
+        try!(set_fd(err_fd, &mut si.hStdError, false));
 
         let cmd_str = make_command_line(cfg.program, cfg.args);
         let mut pi = zeroed_process_information();
@@ -338,9 +352,9 @@ fn spawn_process_os(cfg: ProcessConfig, in_fd: c_int, out_fd: c_int, err_fd: c_i
             })
         });
 
-        if in_fd != -1 { assert!(CloseHandle(si.hStdInput) != 0); }
-        if out_fd != -1 { assert!(CloseHandle(si.hStdOutput) != 0); }
-        if err_fd != -1 { assert!(CloseHandle(si.hStdError) != 0); }
+        assert!(CloseHandle(si.hStdInput) != 0);
+        assert!(CloseHandle(si.hStdOutput) != 0);
+        assert!(CloseHandle(si.hStdError) != 0);
 
         match create_err {
             Some(err) => return Err(err),
@@ -379,9 +393,9 @@ fn zeroed_startupinfo() -> libc::types::os::arch::extra::STARTUPINFO {
         wShowWindow: 0,
         cbReserved2: 0,
         lpReserved2: ptr::mut_null(),
-        hStdInput: ptr::mut_null(),
-        hStdOutput: ptr::mut_null(),
-        hStdError: ptr::mut_null()
+        hStdInput: libc::INVALID_HANDLE_VALUE as libc::HANDLE,
+        hStdOutput: libc::INVALID_HANDLE_VALUE as libc::HANDLE,
+        hStdError: libc::INVALID_HANDLE_VALUE as libc::HANDLE,
     }
 }
 
@@ -489,6 +503,10 @@ fn spawn_process_os(cfg: ProcessConfig, in_fd: c_int, out_fd: c_int, err_fd: c_i
             let mut input = file::FileDesc::new(pipe.input, true);
             let mut output = file::FileDesc::new(pipe.out, true);
 
+            // We may use this in the child, so perform allocations before the
+            // fork
+            let devnull = "/dev/null".to_c_str();
+
             set_cloexec(output.fd());
 
             let pid = fork();
@@ -563,21 +581,29 @@ fn spawn_process_os(cfg: ProcessConfig, in_fd: c_int, out_fd: c_int, err_fd: c_i
 
             rustrt::rust_unset_sigprocmask();
 
-            if in_fd == -1 {
-                let _ = libc::close(libc::STDIN_FILENO);
-            } else if retry(|| dup2(in_fd, 0)) == -1 {
-                fail(&mut output);
-            }
-            if out_fd == -1 {
-                let _ = libc::close(libc::STDOUT_FILENO);
-            } else if retry(|| dup2(out_fd, 1)) == -1 {
-                fail(&mut output);
-            }
-            if err_fd == -1 {
-                let _ = libc::close(libc::STDERR_FILENO);
-            } else if retry(|| dup2(err_fd, 2)) == -1 {
-                fail(&mut output);
-            }
+            // If a stdio file descriptor is set to be ignored (via a -1 file
+            // descriptor), then 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: c_int, dst: c_int| {
+                let src = if src == -1 {
+                    let flags = if dst == libc::STDIN_FILENO {
+                        libc::O_RDONLY
+                    } else {
+                        libc::O_RDWR
+                    };
+                    devnull.with_ref(|p| libc::open(p, flags, 0))
+                } else {
+                    src
+                };
+                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
             for fd in range(3, getdtablesize()).rev() {
                 if fd != output.fd() {

src/test/run-pass/issue-14456.rs

@@ -0,0 +1,55 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(phase)]
+
+#[phase(syntax, link)]
+extern crate green;
+extern crate native;
+
+use std::io::process;
+use std::io::Command;
+use std::io;
+use std::os;
+
+green_start!(main)
+
+fn main() {
+    let args = os::args();
+    if args.len() > 1 && args.get(1).as_slice() == "child" {
+        return child()
+    }
+
+    test();
+
+    let (tx, rx) = channel();
+    native::task::spawn(proc() {
+        tx.send(test());
+    });
+    rx.recv();
+
+}
+
+fn child() {
+    io::stdout().write_line("foo").unwrap();
+    io::stderr().write_line("bar").unwrap();
+    assert_eq!(io::stdin().read_line().err().unwrap().kind, io::EndOfFile);
+}
+
+fn test() {
+    let args = os::args();
+    let mut p = Command::new(args.get(0).as_slice()).arg("child")
+                                     .stdin(process::Ignored)
+                                     .stdout(process::Ignored)
+                                     .stderr(process::Ignored)
+                                     .spawn().unwrap();
+    assert!(p.wait().unwrap().success());
+}
+