diff --git a/src/libstd/sys/windows/c.rs b/src/libstd/sys/windows/c.rs index d1c404195bc68..910d802f05902 100644 --- a/src/libstd/sys/windows/c.rs +++ b/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; diff --git a/src/libstd/sys/windows/pipe.rs b/src/libstd/sys/windows/pipe.rs index 1eb1730547642..8073473f7ff5b 100644 --- a/src/libstd/sys/windows/pipe.rs +++ b/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 { // 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::>(); + 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() }, + }) } } diff --git a/src/libstd/sys/windows/process.rs b/src/libstd/sys/windows/process.rs index 969de6b85a6aa..7dc8959e1b683 100644 --- a/src/libstd/sys/windows/process.rs +++ b/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) => { diff --git a/src/test/run-pass/stdio-is-blocking.rs b/src/test/run-pass/stdio-is-blocking.rs new file mode 100644 index 0000000000000..74170ca6506ec --- /dev/null +++ b/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 or the MIT license +// , 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::>(); + 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::>(); + + 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(); +}