Auto merge of #26159 - alexcrichton:tweak-process-lowering-raising, r=brson

* Slate these features to be stable in 1.2 instead of 1.1 (not being backported)
* Have the `FromRawFd` implementations follow the contract of the `FromRawFd`
  trait by taking ownership of the primitive specified.
* Refactor the implementations slightly to remove the `unreachable!` blocks as
  well as separating the stdio representation of `std::process` from
  `std::sys::process`.

cc #25494

Author: bors

src/libstd/process.rs

@@ -243,13 +243,15 @@ impl Command {
 
     fn spawn_inner(&self, default_io: StdioImp) -> io::Result<Child> {
         let default_io = Stdio(default_io);
-        let (their_stdin, our_stdin) = try!(
+
+        // 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) = try!(
+        let (their_stdout, our_stdout, _drop_later) = try!(
             setup_io(self.stdout.as_ref().unwrap_or(&default_io), false)
         );
-        let (their_stderr, our_stderr) = try!(
+        let (their_stderr, our_stderr, _drop_later) = try!(
             setup_io(self.stderr.as_ref().unwrap_or(&default_io), false)
         );
 
@@ -271,7 +273,7 @@ impl Command {
     /// 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::Raw(imp::Stdio::Inherit))
+        self.spawn_inner(StdioImp::Inherit)
     }
 
     /// Executes the command as a child process, waiting for it to finish and
@@ -341,19 +343,30 @@ 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>)>
+            -> 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::Piped(reader), Some(writer))
+                (imp::Stdio::Raw(reader.raw()), Some(writer), Some(reader))
             } else {
-                (imp::Stdio::Piped(writer), Some(reader))
+                (imp::Stdio::Raw(writer.raw()), Some(reader), Some(writer))
             }
         }
-        StdioImp::Raw(ref raw) => (raw.clone_if_copy(), None),
+        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),
     })
 }
 
@@ -379,7 +392,9 @@ pub struct Stdio(StdioImp);
 // The internal enum for stdio setup; see below for descriptions.
 enum StdioImp {
     MakePipe,
-    Raw(imp::Stdio),
+    Raw(imp::RawStdio),
+    Inherit,
+    None,
 }
 
 impl Stdio {
@@ -389,16 +404,16 @@ impl Stdio {
 
     /// The child inherits from the corresponding parent descriptor.
     #[stable(feature = "process", since = "1.0.0")]
-    pub fn inherit() -> Stdio { Stdio(StdioImp::Raw(imp::Stdio::Inherit)) }
+    pub fn inherit() -> Stdio { Stdio(StdioImp::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::Raw(imp::Stdio::None)) }
+    pub fn null() -> Stdio { Stdio(StdioImp::None) }
 }
 
-impl FromInner<imp::Stdio> for Stdio {
-    fn from_inner(inner: imp::Stdio) -> Stdio {
+impl FromInner<imp::RawStdio> for Stdio {
+    fn from_inner(inner: imp::RawStdio) -> Stdio {
         Stdio(StdioImp::Raw(inner))
     }
 }

src/libstd/sys/unix/ext/process.rs

@@ -65,46 +65,28 @@ impl ExitStatusExt for process::ExitStatus {
     }
 }
 
-#[stable(feature = "from_raw_os", since = "1.1.0")]
+#[stable(feature = "process_extensions", since = "1.2.0")]
 impl FromRawFd for process::Stdio {
-    /// Creates a new instance of `Stdio` from the raw underlying file
-    /// descriptor.
-    ///
-    /// When this `Stdio` is used as an I/O handle for a child process the given
-    /// file descriptor will be `dup`d into the destination file descriptor in
-    /// the child process.
-    ///
-    /// Note that this function **does not** take ownership of the file
-    /// descriptor provided and it will **not** be closed when `Stdio` goes out
-    /// of scope. As a result this method is unsafe because due to the lack of
-    /// knowledge about the lifetime of the provided file descriptor, this could
-    /// cause another I/O primitive's ownership property of its file descriptor
-    /// to be violated.
-    ///
-    /// Also note that this file descriptor may be used multiple times to spawn
-    /// processes. For example the `Command::spawn` function could be called
-    /// more than once to spawn more than one process sharing this file
-    /// descriptor.
     unsafe fn from_raw_fd(fd: RawFd) -> process::Stdio {
-        process::Stdio::from_inner(sys::process::Stdio::Fd(fd))
+        process::Stdio::from_inner(sys::fd::FileDesc::new(fd))
     }
 }
 
-#[stable(feature = "from_raw_os", since = "1.1.0")]
+#[stable(feature = "process_extensions", since = "1.2.0")]
 impl AsRawFd for process::ChildStdin {
     fn as_raw_fd(&self) -> RawFd {
         self.as_inner().fd().raw()
     }
 }
 
-#[stable(feature = "from_raw_os", since = "1.1.0")]
+#[stable(feature = "process_extensions", since = "1.2.0")]
 impl AsRawFd for process::ChildStdout {
     fn as_raw_fd(&self) -> RawFd {
         self.as_inner().fd().raw()
     }
 }
 
-#[stable(feature = "from_raw_os", since = "1.1.0")]
+#[stable(feature = "process_extensions", since = "1.2.0")]
 impl AsRawFd for process::ChildStderr {
     fn as_raw_fd(&self) -> RawFd {
         self.as_inner().fd().raw()

src/libstd/sys/unix/fs.rs

@@ -283,8 +283,6 @@ impl File {
         Ok(File(fd))
     }
 
-    pub fn into_fd(self) -> FileDesc { self.0 }
-
     pub fn file_attr(&self) -> io::Result<FileAttr> {
         let mut stat: raw::stat = unsafe { mem::zeroed() };
         try!(cvt(unsafe {

src/libstd/sys/unix/pipe.rs

@@ -44,6 +44,6 @@ impl AnonPipe {
         self.0.write(buf)
     }
 
+    pub fn raw(&self) -> libc::c_int { self.0.raw() }
     pub fn fd(&self) -> &FileDesc { &self.0 }
-    pub fn into_fd(self) -> FileDesc { self.0 }
 }

src/libstd/sys/unix/process.rs

@@ -18,9 +18,10 @@ use fmt;
 use io::{self, Error, ErrorKind};
 use libc::{self, pid_t, c_void, c_int, gid_t, uid_t};
 use ptr;
+use sys::fd::FileDesc;
+use sys::fs::{File, OpenOptions};
 use sys::pipe::AnonPipe;
 use sys::{self, c, cvt, cvt_r};
-use sys::fs::{File, OpenOptions};
 
 ////////////////////////////////////////////////////////////////////////////////
 // Command
@@ -121,11 +122,12 @@ pub struct Process {
 
 pub enum Stdio {
     Inherit,
-    Piped(AnonPipe),
     None,
-    Fd(c_int),
+    Raw(c_int),
 }
 
+pub type RawStdio = FileDesc;
+
 const CLOEXEC_MSG_FOOTER: &'static [u8] = b"NOEX";
 
 impl Process {
@@ -252,10 +254,9 @@ impl Process {
         }
 
         let setup = |src: Stdio, dst: c_int| {
-            let fd = match src {
-                Stdio::Inherit => return true,
-                Stdio::Fd(fd) => return cvt_r(|| libc::dup2(fd, dst)).is_ok(),
-                Stdio::Piped(pipe) => pipe.into_fd(),
+            match src {
+                Stdio::Inherit => true,
+                Stdio::Raw(fd) => cvt_r(|| libc::dup2(fd, dst)).is_ok(),
 
                 // If a stdio file descriptor is set to be ignored, we open up
                 // /dev/null into that file descriptor. Otherwise, the first
@@ -269,13 +270,12 @@ impl Process {
                     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()
+                        cvt_r(|| libc::dup2(f.fd().raw(), dst)).is_ok()
                     } else {
-                        return false
+                        false
                     }
                 }
-            };
-            cvt_r(|| libc::dup2(fd.raw(), dst)).is_ok()
+            }
         };
 
         if !setup(in_fd, libc::STDIN_FILENO) { fail(&mut output) }
@@ -418,14 +418,3 @@ fn translate_status(status: c_int) -> ExitStatus {
         ExitStatus::Signal(imp::WTERMSIG(status))
     }
 }
-
-impl Stdio {
-    pub fn clone_if_copy(&self) -> Stdio {
-        match *self {
-            Stdio::Inherit => Stdio::Inherit,
-            Stdio::None => Stdio::None,
-            Stdio::Fd(fd) => Stdio::Fd(fd),
-            Stdio::Piped(_) => unreachable!(),
-        }
-    }
-}

src/libstd/sys/windows/ext/process.rs

@@ -10,58 +10,43 @@
 
 //! Extensions to `std::process` for Windows.
 
-#![stable(feature = "from_raw_os", since = "1.1.0")]
+#![stable(feature = "process_extensions", since = "1.2.0")]
 
 use os::windows::io::{FromRawHandle, RawHandle, AsRawHandle};
 use process;
 use sys;
 use sys_common::{AsInner, FromInner};
 
-#[stable(feature = "from_raw_os", since = "1.1.0")]
+#[stable(feature = "process_extensions", since = "1.2.0")]
 impl FromRawHandle for process::Stdio {
-    /// Creates a new instance of `Stdio` from the raw underlying handle.
-    ///
-    /// When this `Stdio` is used as an I/O handle for a child process the given
-    /// handle will be duplicated via `DuplicateHandle` to ensure that the
-    /// handle has the correct permissions to cross the process boundary.
-    ///
-    /// Note that this function **does not** take ownership of the handle
-    /// provided and it will **not** be closed when `Stdio` goes out of scope.
-    /// As a result this method is unsafe because due to the lack of knowledge
-    /// about the lifetime of the provided handle, this could cause another I/O
-    /// primitive's ownership property of its handle to be violated.
-    ///
-    /// Also note that this handle may be used multiple times to spawn
-    /// processes. For example the `Command::spawn` function could be called
-    /// more than once to spawn more than one process sharing this handle.
     unsafe fn from_raw_handle(handle: RawHandle) -> process::Stdio {
-        let handle = sys::handle::RawHandle::new(handle as *mut _);
-        process::Stdio::from_inner(sys::process::Stdio::Handle(handle))
+        let handle = sys::handle::Handle::new(handle as *mut _);
+        process::Stdio::from_inner(handle)
     }
 }
 
-#[stable(feature = "from_raw_os", since = "1.1.0")]
+#[stable(feature = "process_extensions", since = "1.2.0")]
 impl AsRawHandle for process::Child {
     fn as_raw_handle(&self) -> RawHandle {
         self.as_inner().handle().raw() as *mut _
     }
 }
 
-#[stable(feature = "from_raw_os", since = "1.1.0")]
+#[stable(feature = "process_extensions", since = "1.2.0")]
 impl AsRawHandle for process::ChildStdin {
     fn as_raw_handle(&self) -> RawHandle {
         self.as_inner().handle().raw() as *mut _
     }
 }
 
-#[stable(feature = "from_raw_os", since = "1.1.0")]
+#[stable(feature = "process_extensions", since = "1.2.0")]
 impl AsRawHandle for process::ChildStdout {
     fn as_raw_handle(&self) -> RawHandle {
         self.as_inner().handle().raw() as *mut _
     }
 }
 
-#[stable(feature = "from_raw_os", since = "1.1.0")]
+#[stable(feature = "process_extensions", since = "1.2.0")]
 impl AsRawHandle for process::ChildStderr {
     fn as_raw_handle(&self) -> RawHandle {
         self.as_inner().handle().raw() as *mut _

src/libstd/sys/windows/pipe.rs

@@ -38,6 +38,8 @@ pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
 impl AnonPipe {
     pub fn handle(&self) -> &Handle { &self.inner }
 
+    pub fn raw(&self) -> libc::HANDLE { self.inner.raw() }
+
     pub fn read(&self, buf: &mut [u8]) -> io::Result<usize> {
         self.inner.read(buf)
     }

src/libstd/sys/windows/process.rs

@@ -28,7 +28,6 @@ use sync::StaticMutex;
 use sys::c;
 use sys::fs::{OpenOptions, File};
 use sys::handle::{Handle, RawHandle};
-use sys::pipe::AnonPipe;
 use sys::stdio;
 use sys::{self, cvt};
 use sys_common::{AsInner, FromInner};
@@ -107,11 +106,12 @@ pub struct Process {
 
 pub enum Stdio {
     Inherit,
-    Piped(AnonPipe),
     None,
-    Handle(RawHandle),
+    Raw(libc::HANDLE),
 }
 
+pub type RawStdio = Handle;
+
 impl Process {
     pub fn spawn(cfg: &Command,
                  in_handle: Stdio,
@@ -356,15 +356,6 @@ fn make_dirp(d: Option<&OsString>) -> (*const u16, Vec<u16>) {
 }
 
 impl Stdio {
-    pub fn clone_if_copy(&self) -> Stdio {
-        match *self {
-            Stdio::Inherit => Stdio::Inherit,
-            Stdio::None => Stdio::None,
-            Stdio::Handle(handle) => Stdio::Handle(handle),
-            Stdio::Piped(_) => unreachable!(),
-        }
-    }
-
     fn to_handle(&self, stdio_id: libc::DWORD) -> io::Result<Handle> {
         use libc::DUPLICATE_SAME_ACCESS;
 
@@ -374,11 +365,8 @@ impl Stdio {
                     io.handle().duplicate(0, true, DUPLICATE_SAME_ACCESS)
                 })
             }
-            Stdio::Handle(ref handle) => {
-                handle.duplicate(0, true, DUPLICATE_SAME_ACCESS)
-            }
-            Stdio::Piped(ref pipe) => {
-                pipe.handle().duplicate(0, true, DUPLICATE_SAME_ACCESS)
+            Stdio::Raw(handle) => {
+                RawHandle::new(handle).duplicate(0, true, DUPLICATE_SAME_ACCESS)
             }
 
             // Similarly to unix, we don't actually leave holes for the