Auto merge of #31409 - alexcrichton:command-exec, r=aturon

These commits are an implementation of https://github.com/rust-lang/rfcs/pull/1359 which is tracked via https://github.com/rust-lang/rust/issues/31398. The `before_exec` implementation fit easily with the current process spawning framework we have, but unfortunately the `exec` implementation required a bit of a larger refactoring. The stdio handles were all largely managed as implementation details of `std::process` and the `exec` function lived in `std::sys`, so the two didn't have access to one another.

I took this as a sign that a deeper refactoring was necessary, and I personally feel that the end result is cleaner for both Windows and Unix. The commits should be separated nicely for reviewing (or all at once if you're feeling ambitious), but the changes made here were:

* The process spawning on Unix was refactored in to a pre-exec and post-exec function. The post-exec function isn't allowed to do any allocations of any form, and management of transmitting errors back to the parent is managed by the pre-exec function (as it's the one that actually forks).
* Some management of the exit status was pushed into platform-specific modules. On Unix we must cache the return value of `wait` as the pid is consumed after we wait on it, but on Windows we can just keep querying the system because the handle stays valid.
* The `Stdio::None` variant was renamed to `Stdio::Null` to better reflect what it's doing.
* The global lock on `CreateProcess` is now correctly positioned to avoid unintended inheritance of pipe handles that other threads are sending to their child processes. After a more careful reading of the article referenced the race is not in `CreateProcess` itself, but rather the property that handles are unintentionally shared.
* All stdio management now happens in platform-specific modules. This provides a cleaner implementation/interpretation for `FromFraw{Fd,Handle}` for each platform as well as a cleaner transition from a configuration to what-to-do once we actually need to do the spawn.

With these refactorings in place, implementing `before_exec` and `exec` ended up both being pretty trivial! (each in their own commit)

1 files changed, 52 insertions, 130 deletions

diff --git a/src/libstd/process.rs b/src/libstd/process.rs
index a8da11420d8..8db8ad324be 100644
--- a/src/libstd/process.rs
+++ b/src/libstd/process.rs
@@ -11,17 +11,16 @@
 //! Working with processes.
 
 #![stable(feature = "process", since = "1.0.0")]
-#![allow(non_upper_case_globals)]
 
 use prelude::v1::*;
 use io::prelude::*;
 
 use ffi::OsStr;
 use fmt;
-use io::{self, Error, ErrorKind};
-use path;
+use io;
+use path::Path;
 use str;
-use sys::pipe::{self, AnonPipe};
+use sys::pipe::AnonPipe;
 use sys::process as imp;
 use sys_common::{AsInner, AsInnerMut, FromInner, IntoInner};
 use thread::{self, JoinHandle};
@@ -61,9 +60,6 @@ use thread::{self, JoinHandle};
 pub struct Child {
     handle: imp::Process,
 
-    /// None until wait() or wait_with_output() is called.
-    status: Option<imp::ExitStatus>,
-
     /// The handle for writing to the child's stdin, if it has been captured
     #[stable(feature = "process", since = "1.0.0")]
     pub stdin: Option<ChildStdin>,
@@ -81,6 +77,17 @@ impl AsInner<imp::Process> for Child {
     fn as_inner(&self) -> &imp::Process { &self.handle }
 }
 
+impl FromInner<(imp::Process, imp::StdioPipes)> for Child {
+    fn from_inner((handle, io): (imp::Process, imp::StdioPipes)) -> Child {
+        Child {
+            handle: handle,
+            stdin: io.stdin.map(ChildStdin::from_inner),
+            stdout: io.stdout.map(ChildStdout::from_inner),
+            stderr: io.stderr.map(ChildStderr::from_inner),
+        }
+    }
+}
+
 impl IntoInner<imp::Process> for Child {
     fn into_inner(self) -> imp::Process { self.handle }
 }
@@ -110,6 +117,12 @@ impl IntoInner<AnonPipe> for ChildStdin {
     fn into_inner(self) -> AnonPipe { self.inner }
 }
 
+impl FromInner<AnonPipe> for ChildStdin {
+    fn from_inner(pipe: AnonPipe) -> ChildStdin {
+        ChildStdin { inner: pipe }
+    }
+}
+
 /// A handle to a child process's stdout
 #[stable(feature = "process", since = "1.0.0")]
 pub struct ChildStdout {
@@ -131,6 +144,12 @@ impl IntoInner<AnonPipe> for ChildStdout {
     fn into_inner(self) -> AnonPipe { self.inner }
 }
 
+impl FromInner<AnonPipe> for ChildStdout {
+    fn from_inner(pipe: AnonPipe) -> ChildStdout {
+        ChildStdout { inner: pipe }
+    }
+}
+
 /// A handle to a child process's stderr
 #[stable(feature = "process", since = "1.0.0")]
 pub struct ChildStderr {
@@ -152,6 +171,12 @@ impl IntoInner<AnonPipe> for ChildStderr {
     fn into_inner(self) -> AnonPipe { self.inner }
 }
 
+impl FromInner<AnonPipe> for ChildStderr {
+    fn from_inner(pipe: AnonPipe) -> ChildStderr {
+        ChildStderr { inner: pipe }
+    }
+}
+
 /// The `Command` type acts as a process builder, providing fine-grained control
 /// over how a new process should be spawned. A default configuration can be
 /// generated using `Command::new(program)`, where `program` gives a path to the
@@ -171,11 +196,6 @@ impl IntoInner<AnonPipe> for ChildStderr {
 #[stable(feature = "process", since = "1.0.0")]
 pub struct Command {
     inner: imp::Command,
-
-    // Details explained in the builder methods
-    stdin: Option<Stdio>,
-    stdout: Option<Stdio>,
-    stderr: Option<Stdio>,
 }
 
 impl Command {
@@ -191,12 +211,7 @@ impl Command {
     /// otherwise configure the process.
     #[stable(feature = "process", since = "1.0.0")]
     pub fn new<S: AsRef<OsStr>>(program: S) -> Command {
-        Command {
-            inner: imp::Command::new(program.as_ref()),
-            stdin: None,
-            stdout: None,
-            stderr: None,
-        }
+        Command { inner: imp::Command::new(program.as_ref()) }
     }
 
     /// Add an argument to pass to the program.
@@ -209,7 +224,9 @@ impl Command {
     /// Add multiple arguments to pass to the program.
     #[stable(feature = "process", since = "1.0.0")]
     pub fn args<S: AsRef<OsStr>>(&mut self, args: &[S]) -> &mut Command {
-        self.inner.args(args.iter().map(AsRef::as_ref));
+        for arg in args {
+            self.arg(arg.as_ref());
+        }
         self
     }
 
@@ -241,7 +258,7 @@ impl Command {
 
     /// Sets the working directory for the child process.
     #[stable(feature = "process", since = "1.0.0")]
-    pub fn current_dir<P: AsRef<path::Path>>(&mut self, dir: P) -> &mut Command {
+    pub fn current_dir<P: AsRef<Path>>(&mut self, dir: P) -> &mut Command {
         self.inner.cwd(dir.as_ref().as_ref());
         self
     }
@@ -249,57 +266,30 @@ impl Command {
     /// Configuration for the child process's stdin handle (file descriptor 0).
     #[stable(feature = "process", since = "1.0.0")]
     pub fn stdin(&mut self, cfg: Stdio) -> &mut Command {
-        self.stdin = Some(cfg);
+        self.inner.stdin(cfg.0);
         self
     }
 
     /// Configuration for the child process's stdout handle (file descriptor 1).
     #[stable(feature = "process", since = "1.0.0")]
     pub fn stdout(&mut self, cfg: Stdio) -> &mut Command {
-        self.stdout = Some(cfg);
+        self.inner.stdout(cfg.0);
         self
     }
 
     /// Configuration for the child process's stderr handle (file descriptor 2).
     #[stable(feature = "process", since = "1.0.0")]
     pub fn stderr(&mut self, cfg: Stdio) -> &mut Command {
-        self.stderr = Some(cfg);
+        self.inner.stderr(cfg.0);
         self
     }
 
-    fn spawn_inner(&self, default_io: StdioImp) -> io::Result<Child> {
-        let default_io = Stdio(default_io);
-
-        // See comment on `setup_io` for what `_drop_later` is.
-        let (their_stdin, our_stdin, _drop_later) = try!(
-            setup_io(self.stdin.as_ref().unwrap_or(&default_io), true)
-        );
-        let (their_stdout, our_stdout, _drop_later) = try!(
-            setup_io(self.stdout.as_ref().unwrap_or(&default_io), false)
-        );
-        let (their_stderr, our_stderr, _drop_later) = try!(
-            setup_io(self.stderr.as_ref().unwrap_or(&default_io), false)
-        );
-
-        match imp::Process::spawn(&self.inner, their_stdin, their_stdout,
-                                  their_stderr) {
-            Err(e) => Err(e),
-            Ok(handle) => Ok(Child {
-                handle: handle,
-                status: None,
-                stdin: our_stdin.map(|fd| ChildStdin { inner: fd }),
-                stdout: our_stdout.map(|fd| ChildStdout { inner: fd }),
-                stderr: our_stderr.map(|fd| ChildStderr { inner: fd }),
-            })
-        }
-    }
-
     /// Executes the command as a child process, returning a handle to it.
     ///
     /// By default, stdin, stdout and stderr are inherited from the parent.
     #[stable(feature = "process", since = "1.0.0")]
     pub fn spawn(&mut self) -> io::Result<Child> {
-        self.spawn_inner(StdioImp::Inherit)
+        self.inner.spawn(imp::Stdio::Inherit).map(Child::from_inner)
     }
 
     /// Executes the command as a child process, waiting for it to finish and
@@ -322,7 +312,8 @@ impl Command {
     /// ```
     #[stable(feature = "process", since = "1.0.0")]
     pub fn output(&mut self) -> io::Result<Output> {
-        self.spawn_inner(StdioImp::MakePipe).and_then(|p| p.wait_with_output())
+        self.inner.spawn(imp::Stdio::MakePipe).map(Child::from_inner)
+            .and_then(|p| p.wait_with_output())
     }
 
     /// Executes a command as a child process, waiting for it to finish and
@@ -365,33 +356,6 @@ impl AsInnerMut<imp::Command> for Command {
     fn as_inner_mut(&mut self) -> &mut imp::Command { &mut self.inner }
 }
 
-// Takes a `Stdio` configuration (this module) and whether the to-be-owned
-// handle will be readable.
-//
-// Returns a triple of (stdio to spawn with, stdio to store, stdio to drop). The
-// stdio to spawn with is passed down to the `sys` module and indicates how the
-// stdio stream should be set up. The "stdio to store" is an object which
-// should be returned in the `Child` that makes its way out. The "stdio to drop"
-// represents the raw value of "stdio to spawn with", but is the owned variant
-// for it. This needs to be dropped after the child spawns
-fn setup_io(io: &Stdio, readable: bool)
-            -> io::Result<(imp::Stdio, Option<AnonPipe>, Option<AnonPipe>)>
-{
-    Ok(match io.0 {
-        StdioImp::MakePipe => {
-            let (reader, writer) = try!(pipe::anon_pipe());
-            if readable {
-                (imp::Stdio::Raw(reader.raw()), Some(writer), Some(reader))
-            } else {
-                (imp::Stdio::Raw(writer.raw()), Some(reader), Some(writer))
-            }
-        }
-        StdioImp::Raw(ref owned) => (imp::Stdio::Raw(owned.raw()), None, None),
-        StdioImp::Inherit => (imp::Stdio::Inherit, None, None),
-        StdioImp::None => (imp::Stdio::None, None, None),
-    })
-}
-
 /// The output of a finished process.
 #[derive(PartialEq, Eq, Clone)]
 #[stable(feature = "process", since = "1.0.0")]
@@ -435,34 +399,26 @@ impl fmt::Debug for Output {
 
 /// Describes what to do with a standard I/O stream for a child process.
 #[stable(feature = "process", since = "1.0.0")]
-pub struct Stdio(StdioImp);
-
-// The internal enum for stdio setup; see below for descriptions.
-enum StdioImp {
-    MakePipe,
-    Raw(imp::RawStdio),
-    Inherit,
-    None,
-}
+pub struct Stdio(imp::Stdio);
 
 impl Stdio {
     /// A new pipe should be arranged to connect the parent and child processes.
     #[stable(feature = "process", since = "1.0.0")]
-    pub fn piped() -> Stdio { Stdio(StdioImp::MakePipe) }
+    pub fn piped() -> Stdio { Stdio(imp::Stdio::MakePipe) }
 
     /// The child inherits from the corresponding parent descriptor.
     #[stable(feature = "process", since = "1.0.0")]
-    pub fn inherit() -> Stdio { Stdio(StdioImp::Inherit) }
+    pub fn inherit() -> Stdio { Stdio(imp::Stdio::Inherit) }
 
     /// This stream will be ignored. This is the equivalent of attaching the
     /// stream to `/dev/null`
     #[stable(feature = "process", since = "1.0.0")]
-    pub fn null() -> Stdio { Stdio(StdioImp::None) }
+    pub fn null() -> Stdio { Stdio(imp::Stdio::Null) }
 }
 
-impl FromInner<imp::RawStdio> for Stdio {
-    fn from_inner(inner: imp::RawStdio) -> Stdio {
-        Stdio(StdioImp::Raw(inner))
+impl FromInner<imp::Stdio> for Stdio {
+    fn from_inner(inner: imp::Stdio) -> Stdio {
+        Stdio(inner)
     }
 }
 
@@ -506,34 +462,7 @@ impl Child {
     /// SIGKILL on unix platforms.
     #[stable(feature = "process", since = "1.0.0")]
     pub fn kill(&mut self) -> io::Result<()> {
-        #[cfg(unix)] fn collect_status(p: &mut Child) {
-            // On Linux (and possibly other unices), a process that has exited will
-            // continue to accept signals because it is "defunct". The delivery of
-            // signals will only fail once the child has been reaped. For this
-            // reason, if the process hasn't exited yet, then we attempt to collect
-            // their status with WNOHANG.
-            if p.status.is_none() {
-                match p.handle.try_wait() {
-                    Some(status) => { p.status = Some(status); }
-                    None => {}
-                }
-            }
-        }
-        #[cfg(windows)] fn collect_status(_p: &mut Child) {}
-
-        collect_status(self);
-
-        // if the process has finished, and therefore had waitpid called,
-        // and we kill it, then on unix we might ending up killing a
-        // newer process that happens to have the same (re-used) id
-        if self.status.is_some() {
-            return Err(Error::new(
-                ErrorKind::InvalidInput,
-                "invalid argument: can't kill an exited process",
-            ))
-        }
-
-        unsafe { self.handle.kill() }
+        self.handle.kill()
     }
 
     /// Returns the OS-assigned process identifier associated with this child.
@@ -553,14 +482,7 @@ impl Child {
     #[stable(feature = "process", since = "1.0.0")]
     pub fn wait(&mut self) -> io::Result<ExitStatus> {
         drop(self.stdin.take());
-        match self.status {
-            Some(code) => Ok(ExitStatus(code)),
-            None => {
-                let status = try!(self.handle.wait());
-                self.status = Some(status);
-                Ok(ExitStatus(status))
-            }
-        }
+        self.handle.wait().map(ExitStatus)
     }
 
     /// Simultaneously waits for the child to exit and collect all remaining