Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read of asynchronous pipe without synchronization #95411

Closed
ChrisDenton opened this issue Mar 28, 2022 · 0 comments · Fixed by #95467
Closed

Read of asynchronous pipe without synchronization #95411

ChrisDenton opened this issue Mar 28, 2022 · 0 comments · Fixed by #95467
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@ChrisDenton
Copy link
Member

In src/sys/windows/pipe.rs a pipe is opened for asynchronous access (aka "overlapped mode").

/// 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, their_handle_inheritable: bool) -> io::Result<Pipes> {

However the read method for pipes is:

pub fn read(&self, buf: &mut [u8]) -> io::Result<usize> {
self.inner.read(buf)
}

Where inner is a Handle whose read method does not wait for the buffer to be filled:

pub fn read(&self, buf: &mut [u8]) -> io::Result<usize> {
let mut read = 0;
let len = cmp::min(buf.len(), <c::DWORD>::MAX as usize) as c::DWORD;
let res = cvt(unsafe {
c::ReadFile(
self.as_handle(),
buf.as_mut_ptr() as c::LPVOID,
len,
&mut read,
ptr::null_mut(),
)
});
match res {
Ok(_) => Ok(read as usize),
// The special treatment of BrokenPipe is to deal with Windows
// pipe semantics, which yields this error when *reading* from
// a pipe after the other end has closed; we interpret that as
// EOF on the pipe.
Err(ref e) if e.kind() == ErrorKind::BrokenPipe => Ok(0),
Err(e) => Err(e),
}
}

As far as I know this pipe is only used for process::ChildStd* so we (probably) get away with it in typical usage. Also the read2 method below this does the right thing by synchronizing reads.

Found while investigating #81357

@rustbot label +T-libs +O-windows

@ChrisDenton ChrisDenton added the C-bug Category: This is a bug. label Mar 28, 2022
@rustbot rustbot added O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 28, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 4, 2022
…triplett

Windows: Synchronize asynchronous pipe reads and writes

On Windows, the pipes used for spawned processes are opened for asynchronous access but `read` and `write` are done using the standard methods that assume synchronous access. This means that the buffer (and variables on the stack) may be read/written to after the function returns.

This PR ensures reads/writes complete before returning. Note that this only applies to pipes we create and does not affect the standard file read/write methods.

Fixes rust-lang#95411
@bors bors closed this as completed in 4cbc003 Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants