Auto merge of #38835 - alexcrichton:less-overlapped, r=brson

std: Don't pass overlapped handles to processes

This commit fixes a mistake introduced in #31618 where overlapped handles were
leaked to child processes on Windows. On Windows once a handle is in overlapped
mode it should always have I/O executed with an instance of `OVERLAPPED`. Most
child processes, however, are not prepared to have their stdio handles in
overlapped mode as they don't use `OVERLAPPED` on reads/writes to the handle.

Now we haven't had any odd behavior in Rust up to this point, and the original
bug was introduced almost a year ago. I believe this is because it turns out
that if you *don't* pass an `OVERLAPPED` then the system will [supply one for
you][link]. In this case everything will go awry if you concurrently operate on
the handle. In Rust, however, the stdio handles are always locked, and there's
no way to not use them unlocked in libstd. Due to that change we've always had
synchronized access to these handles, which means that Rust programs typically
"just work".

Conversely, though, this commit fixes the test case included, which exhibits
behavior that other programs Rust spawns may attempt to execute. Namely, the
stdio handles may be concurrently used and having them in overlapped mode wreaks
havoc.

[link]: https://blogs.msdn.microsoft.com/oldnewthing/20121012-00/?p=6343

Closes #38811

src/libstd/sys/windows/c.rs

@@ -282,6 +282,7 @@ pub const EXCEPTION_STACK_OVERFLOW: DWORD = 0xc00000fd;
 pub const EXCEPTION_MAXIMUM_PARAMETERS: usize = 15;
 
 pub const PIPE_ACCESS_INBOUND: DWORD = 0x00000001;
+pub const PIPE_ACCESS_OUTBOUND: DWORD = 0x00000002;
 pub const FILE_FLAG_FIRST_PIPE_INSTANCE: DWORD = 0x00080000;
 pub const FILE_FLAG_OVERLAPPED: DWORD = 0x40000000;
 pub const PIPE_WAIT: DWORD = 0x00000000;

src/libstd/sys/windows/pipe.rs

@@ -29,18 +29,43 @@ pub struct AnonPipe {
     inner: Handle,
 }
 
-pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
+pub struct Pipes {
+    pub ours: AnonPipe,
+    pub theirs: AnonPipe,
+}
+
+/// Although this looks similar to `anon_pipe` in the Unix module it's actually
+/// subtly different. Here we'll return two pipes in the `Pipes` return value,
+/// but one is intended for "us" where as the other is intended for "someone
+/// else".
+///
+/// Currently the only use case for this function is pipes for stdio on
+/// processes in the standard library, so "ours" is the one that'll stay in our
+/// process whereas "theirs" will be inherited to a child.
+///
+/// The ours/theirs pipes are *not* specifically readable or writable. Each
+/// one only supports a read or a write, but which is which depends on the
+/// boolean flag given. If `ours_readable` is true then `ours` is readable where
+/// `theirs` is writable. Conversely if `ours_readable` is false then `ours` is
+/// writable where `theirs` is readable.
+///
+/// Also note that the `ours` pipe is always a handle opened up in overlapped
+/// mode. This means that technically speaking it should only ever be used
+/// with `OVERLAPPED` instances, but also works out ok if it's only ever used
+/// once at a time (which we do indeed guarantee).
+pub fn anon_pipe(ours_readable: bool) -> io::Result<Pipes> {
     // Note that we specifically do *not* use `CreatePipe` here because
     // unfortunately the anonymous pipes returned do not support overlapped
-    // operations.
-    //
-    // Instead, we create a "hopefully unique" name and create a named pipe
-    // which has overlapped operations enabled.
+    // operations. Instead, we create a "hopefully unique" name and create a
+    // named pipe which has overlapped operations enabled.
     //
-    // Once we do this, we connect do it as usual via `CreateFileW`, and then we
-    // return those reader/writer halves.
+    // Once we do this, we connect do it as usual via `CreateFileW`, and then
+    // we return those reader/writer halves. Note that the `ours` pipe return
+    // value is always the named pipe, whereas `theirs` is just the normal file.
+    // This should hopefully shield us from child processes which assume their
+    // stdout is a named pipe, which would indeed be odd!
     unsafe {
-        let reader;
+        let ours;
         let mut name;
         let mut tries = 0;
         let mut reject_remote_clients_flag = c::PIPE_REJECT_REMOTE_CLIENTS;
@@ -54,11 +79,16 @@ pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
                                   .encode_wide()
                                   .chain(Some(0))
                                   .collect::<Vec<_>>();
+            let mut flags = c::FILE_FLAG_FIRST_PIPE_INSTANCE |
+                c::FILE_FLAG_OVERLAPPED;
+            if ours_readable {
+                flags |= c::PIPE_ACCESS_INBOUND;
+            } else {
+                flags |= c::PIPE_ACCESS_OUTBOUND;
+            }
 
             let handle = c::CreateNamedPipeW(wide_name.as_ptr(),
-                                             c::PIPE_ACCESS_INBOUND |
-                                             c::FILE_FLAG_FIRST_PIPE_INSTANCE |
-                                             c::FILE_FLAG_OVERLAPPED,
+                                             flags,
                                              c::PIPE_TYPE_BYTE |
                                              c::PIPE_READMODE_BYTE |
                                              c::PIPE_WAIT |
@@ -101,21 +131,28 @@ pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
                 }
                 return Err(err)
             }
-            reader = Handle::new(handle);
+            ours = Handle::new(handle);
             break
         }
 
-        // Connect to the named pipe we just created in write-only mode (also
-        // overlapped for async I/O below).
+        // Connect to the named pipe we just created. This handle is going to be
+        // returned in `theirs`, so if `ours` is readable we want this to be
+        // writable, otherwise if `ours` is writable we want this to be
+        // readable.
+        //
+        // Additionally we don't enable overlapped mode on this because most
+        // client processes aren't enabled to work with that.
         let mut opts = OpenOptions::new();
-        opts.write(true);
-        opts.read(false);
+        opts.write(ours_readable);
+        opts.read(!ours_readable);
         opts.share_mode(0);
-        opts.attributes(c::FILE_FLAG_OVERLAPPED);
-        let writer = File::open(Path::new(&name), &opts)?;
-        let writer = AnonPipe { inner: writer.into_handle() };
+        let theirs = File::open(Path::new(&name), &opts)?;
+        let theirs = AnonPipe { inner: theirs.into_handle() };
 
-        Ok((AnonPipe { inner: reader }, AnonPipe { inner: writer.into_handle() }))
+        Ok(Pipes {
+            ours: AnonPipe { inner: ours },
+            theirs: AnonPipe { inner: theirs.into_handle() },
+        })
     }
 }
 

src/libstd/sys/windows/process.rs

@@ -264,19 +264,15 @@ impl Stdio {
             }
 
             Stdio::MakePipe => {
-                let (reader, writer) = pipe::anon_pipe()?;
-                let (ours, theirs) = if stdio_id == c::STD_INPUT_HANDLE {
-                    (writer, reader)
-                } else {
-                    (reader, writer)
-                };
-                *pipe = Some(ours);
+                let ours_readable = stdio_id != c::STD_INPUT_HANDLE;
+                let pipes = pipe::anon_pipe(ours_readable)?;
+                *pipe = Some(pipes.ours);
                 cvt(unsafe {
-                    c::SetHandleInformation(theirs.handle().raw(),
+                    c::SetHandleInformation(pipes.theirs.handle().raw(),
                                             c::HANDLE_FLAG_INHERIT,
                                             c::HANDLE_FLAG_INHERIT)
                 })?;
-                Ok(theirs.into_handle())
+                Ok(pipes.theirs.into_handle())
             }
 
             Stdio::Handle(ref handle) => {

src/test/run-pass/stdio-is-blocking.rs

@@ -0,0 +1,90 @@
+// Copyright 2017 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.
+
+use std::env;
+use std::io::prelude::*;
+use std::process::Command;
+use std::thread;
+
+const THREADS: usize = 20;
+const WRITES: usize = 100;
+const WRITE_SIZE: usize = 1024 * 32;
+
+fn main() {
+    let args = env::args().collect::<Vec<_>>();
+    if args.len() == 1 {
+        parent();
+    } else {
+        child();
+    }
+}
+
+fn parent() {
+    let me = env::current_exe().unwrap();
+    let mut cmd = Command::new(me);
+    cmd.arg("run-the-test");
+    let output = cmd.output().unwrap();
+    assert!(output.status.success());
+    assert_eq!(output.stderr.len(), 0);
+    assert_eq!(output.stdout.len(), WRITES * THREADS * WRITE_SIZE);
+    for byte in output.stdout.iter() {
+        assert_eq!(*byte, b'a');
+    }
+}
+
+fn child() {
+    let threads = (0..THREADS).map(|_| {
+        thread::spawn(|| {
+            let buf = [b'a'; WRITE_SIZE];
+            for _ in 0..WRITES {
+                write_all(&buf);
+            }
+        })
+    }).collect::<Vec<_>>();
+
+    for thread in threads {
+        thread.join().unwrap();
+    }
+}
+
+#[cfg(unix)]
+fn write_all(buf: &[u8]) {
+    use std::fs::File;
+    use std::mem;
+    use std::os::unix::prelude::*;
+
+    let mut file = unsafe { File::from_raw_fd(1) };
+    let res = file.write_all(buf);
+    mem::forget(file);
+    res.unwrap();
+}
+
+#[cfg(windows)]
+fn write_all(buf: &[u8]) {
+    use std::fs::File;
+    use std::mem;
+    use std::os::windows::raw::*;
+    use std::os::windows::prelude::*;
+
+    const STD_OUTPUT_HANDLE: u32 = (-11i32) as u32;
+
+    extern "system" {
+        fn GetStdHandle(handle: u32) -> HANDLE;
+    }
+
+    let mut file = unsafe {
+        let handle = GetStdHandle(STD_OUTPUT_HANDLE);
+        assert!(!handle.is_null());
+        File::from_raw_handle(handle)
+    };
+    let res = file.write_all(buf);
+    mem::forget(file);
+    res.unwrap();
+}