Skip to content

Commit 7e38a89

Browse files
borsbors
committed
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
2 parents 373efe8 + 5148918 commit 7e38a89

File tree

4 files changed

+153
-29
lines changed

4 files changed

+153
-29
lines changed

‎src/libstd/sys/windows/c.rs

+1
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,7 @@ pub const EXCEPTION_STACK_OVERFLOW: DWORD = 0xc00000fd;
282282
pub const EXCEPTION_MAXIMUM_PARAMETERS: usize = 15;
283283

284284
pub const PIPE_ACCESS_INBOUND: DWORD = 0x00000001;
285+
pub const PIPE_ACCESS_OUTBOUND: DWORD = 0x00000002;
285286
pub const FILE_FLAG_FIRST_PIPE_INSTANCE: DWORD = 0x00080000;
286287
pub const FILE_FLAG_OVERLAPPED: DWORD = 0x40000000;
287288
pub const PIPE_WAIT: DWORD = 0x00000000;

‎src/libstd/sys/windows/pipe.rs

+57-20
Original file line numberDiff line numberDiff line change
@@ -29,18 +29,43 @@ pub struct AnonPipe {
2929
inner: Handle,
3030
}
3131

32-
pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
32+
pub struct Pipes {
33+
pub ours: AnonPipe,
34+
pub theirs: AnonPipe,
35+
}
36+
37+
/// Although this looks similar to `anon_pipe` in the Unix module it's actually
38+
/// subtly different. Here we'll return two pipes in the `Pipes` return value,
39+
/// but one is intended for "us" where as the other is intended for "someone
40+
/// else".
41+
///
42+
/// Currently the only use case for this function is pipes for stdio on
43+
/// processes in the standard library, so "ours" is the one that'll stay in our
44+
/// process whereas "theirs" will be inherited to a child.
45+
///
46+
/// The ours/theirs pipes are *not* specifically readable or writable. Each
47+
/// one only supports a read or a write, but which is which depends on the
48+
/// boolean flag given. If `ours_readable` is true then `ours` is readable where
49+
/// `theirs` is writable. Conversely if `ours_readable` is false then `ours` is
50+
/// writable where `theirs` is readable.
51+
///
52+
/// Also note that the `ours` pipe is always a handle opened up in overlapped
53+
/// mode. This means that technically speaking it should only ever be used
54+
/// with `OVERLAPPED` instances, but also works out ok if it's only ever used
55+
/// once at a time (which we do indeed guarantee).
56+
pub fn anon_pipe(ours_readable: bool) -> io::Result<Pipes> {
3357
// Note that we specifically do *not* use `CreatePipe` here because
3458
// unfortunately the anonymous pipes returned do not support overlapped
35-
// operations.
36-
//
37-
// Instead, we create a "hopefully unique" name and create a named pipe
38-
// which has overlapped operations enabled.
59+
// operations. Instead, we create a "hopefully unique" name and create a
60+
// named pipe which has overlapped operations enabled.
3961
//
40-
// Once we do this, we connect do it as usual via `CreateFileW`, and then we
41-
// return those reader/writer halves.
62+
// Once we do this, we connect do it as usual via `CreateFileW`, and then
63+
// we return those reader/writer halves. Note that the `ours` pipe return
64+
// value is always the named pipe, whereas `theirs` is just the normal file.
65+
// This should hopefully shield us from child processes which assume their
66+
// stdout is a named pipe, which would indeed be odd!
4267
unsafe {
43-
let reader;
68+
let ours;
4469
let mut name;
4570
let mut tries = 0;
4671
let mut reject_remote_clients_flag = c::PIPE_REJECT_REMOTE_CLIENTS;
@@ -54,11 +79,16 @@ pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
5479
.encode_wide()
5580
.chain(Some(0))
5681
.collect::<Vec<_>>();
82+
let mut flags = c::FILE_FLAG_FIRST_PIPE_INSTANCE |
83+
c::FILE_FLAG_OVERLAPPED;
84+
if ours_readable {
85+
flags |= c::PIPE_ACCESS_INBOUND;
86+
} else {
87+
flags |= c::PIPE_ACCESS_OUTBOUND;
88+
}
5789

5890
let handle = c::CreateNamedPipeW(wide_name.as_ptr(),
59-
c::PIPE_ACCESS_INBOUND |
60-
c::FILE_FLAG_FIRST_PIPE_INSTANCE |
61-
c::FILE_FLAG_OVERLAPPED,
91+
flags,
6292
c::PIPE_TYPE_BYTE |
6393
c::PIPE_READMODE_BYTE |
6494
c::PIPE_WAIT |
@@ -101,21 +131,28 @@ pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> {
101131
}
102132
return Err(err)
103133
}
104-
reader = Handle::new(handle);
134+
ours = Handle::new(handle);
105135
break
106136
}
107137

108-
// Connect to the named pipe we just created in write-only mode (also
109-
// overlapped for async I/O below).
138+
// Connect to the named pipe we just created. This handle is going to be
139+
// returned in `theirs`, so if `ours` is readable we want this to be
140+
// writable, otherwise if `ours` is writable we want this to be
141+
// readable.
142+
//
143+
// Additionally we don't enable overlapped mode on this because most
144+
// client processes aren't enabled to work with that.
110145
let mut opts = OpenOptions::new();
111-
opts.write(true);
112-
opts.read(false);
146+
opts.write(ours_readable);
147+
opts.read(!ours_readable);
113148
opts.share_mode(0);
114-
opts.attributes(c::FILE_FLAG_OVERLAPPED);
115-
let writer = File::open(Path::new(&name), &opts)?;
116-
let writer = AnonPipe { inner: writer.into_handle() };
149+
let theirs = File::open(Path::new(&name), &opts)?;
150+
let theirs = AnonPipe { inner: theirs.into_handle() };
117151

118-
Ok((AnonPipe { inner: reader }, AnonPipe { inner: writer.into_handle() }))
152+
Ok(Pipes {
153+
ours: AnonPipe { inner: ours },
154+
theirs: AnonPipe { inner: theirs.into_handle() },
155+
})
119156
}
120157
}
121158

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

+5-9
Original file line numberDiff line numberDiff line change
@@ -264,19 +264,15 @@ impl Stdio {
264264
}
265265

266266
Stdio::MakePipe => {
267-
let (reader, writer) = pipe::anon_pipe()?;
268-
let (ours, theirs) = if stdio_id == c::STD_INPUT_HANDLE {
269-
(writer, reader)
270-
} else {
271-
(reader, writer)
272-
};
273-
*pipe = Some(ours);
267+
let ours_readable = stdio_id != c::STD_INPUT_HANDLE;
268+
let pipes = pipe::anon_pipe(ours_readable)?;
269+
*pipe = Some(pipes.ours);
274270
cvt(unsafe {
275-
c::SetHandleInformation(theirs.handle().raw(),
271+
c::SetHandleInformation(pipes.theirs.handle().raw(),
276272
c::HANDLE_FLAG_INHERIT,
277273
c::HANDLE_FLAG_INHERIT)
278274
})?;
279-
Ok(theirs.into_handle())
275+
Ok(pipes.theirs.into_handle())
280276
}
281277

282278
Stdio::Handle(ref handle) => {

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

+90
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
use std::env;
12+
use std::io::prelude::*;
13+
use std::process::Command;
14+
use std::thread;
15+
16+
const THREADS: usize = 20;
17+
const WRITES: usize = 100;
18+
const WRITE_SIZE: usize = 1024 * 32;
19+
20+
fn main() {
21+
let args = env::args().collect::<Vec<_>>();
22+
if args.len() == 1 {
23+
parent();
24+
} else {
25+
child();
26+
}
27+
}
28+
29+
fn parent() {
30+
let me = env::current_exe().unwrap();
31+
let mut cmd = Command::new(me);
32+
cmd.arg("run-the-test");
33+
let output = cmd.output().unwrap();
34+
assert!(output.status.success());
35+
assert_eq!(output.stderr.len(), 0);
36+
assert_eq!(output.stdout.len(), WRITES * THREADS * WRITE_SIZE);
37+
for byte in output.stdout.iter() {
38+
assert_eq!(*byte, b'a');
39+
}
40+
}
41+
42+
fn child() {
43+
let threads = (0..THREADS).map(|_| {
44+
thread::spawn(|| {
45+
let buf = [b'a'; WRITE_SIZE];
46+
for _ in 0..WRITES {
47+
write_all(&buf);
48+
}
49+
})
50+
}).collect::<Vec<_>>();
51+
52+
for thread in threads {
53+
thread.join().unwrap();
54+
}
55+
}
56+
57+
#[cfg(unix)]
58+
fn write_all(buf: &[u8]) {
59+
use std::fs::File;
60+
use std::mem;
61+
use std::os::unix::prelude::*;
62+
63+
let mut file = unsafe { File::from_raw_fd(1) };
64+
let res = file.write_all(buf);
65+
mem::forget(file);
66+
res.unwrap();
67+
}
68+
69+
#[cfg(windows)]
70+
fn write_all(buf: &[u8]) {
71+
use std::fs::File;
72+
use std::mem;
73+
use std::os::windows::raw::*;
74+
use std::os::windows::prelude::*;
75+
76+
const STD_OUTPUT_HANDLE: u32 = (-11i32) as u32;
77+
78+
extern "system" {
79+
fn GetStdHandle(handle: u32) -> HANDLE;
80+
}
81+
82+
let mut file = unsafe {
83+
let handle = GetStdHandle(STD_OUTPUT_HANDLE);
84+
assert!(!handle.is_null());
85+
File::from_raw_handle(handle)
86+
};
87+
let res = file.write_all(buf);
88+
mem::forget(file);
89+
res.unwrap();
90+
}

0 commit comments

Comments
 (0)