From 6259670d50b145461f729042be8705444f4d8f78 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Tue, 17 May 2022 18:46:11 -0400 Subject: [PATCH] Revert "Auto merge of #96441 - ChrisDenton:sync-pipes, r=m-ou-se" This reverts commit ddb7fbe8434be481607ae199fe2aee976ee2fc2e, reversing changes made to baaa3b682986879c7784b5733ecea942e9ae7de3. --- library/std/src/os/windows/io/handle.rs | 13 --- library/std/src/sys/windows/c.rs | 6 -- library/std/src/sys/windows/handle.rs | 5 -- library/std/src/sys/windows/pipe.rs | 101 ++++++------------------ library/std/src/sys/windows/process.rs | 38 ++------- 5 files changed, 31 insertions(+), 132 deletions(-) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 0ecac6b447570..90a5b7466fec4 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -204,19 +204,6 @@ impl OwnedHandle { })?; unsafe { Ok(Self::from_raw_handle(ret)) } } - - /// Allow child processes to inherit the handle. - #[cfg(not(target_vendor = "uwp"))] - pub(crate) fn set_inheritable(&self) -> io::Result<()> { - cvt(unsafe { - c::SetHandleInformation( - self.as_raw_handle(), - c::HANDLE_FLAG_INHERIT, - c::HANDLE_FLAG_INHERIT, - ) - })?; - Ok(()) - } } impl TryFrom for OwnedHandle { diff --git a/library/std/src/sys/windows/c.rs b/library/std/src/sys/windows/c.rs index 0692da1d79519..5f14edaf067c0 100644 --- a/library/std/src/sys/windows/c.rs +++ b/library/std/src/sys/windows/c.rs @@ -1022,12 +1022,6 @@ extern "system" { bWaitAll: BOOL, dwMilliseconds: DWORD, ) -> DWORD; - pub fn CreatePipe( - hReadPipe: *mut HANDLE, - hWritePipe: *mut HANDLE, - lpPipeAttributes: *const SECURITY_ATTRIBUTES, - nSize: DWORD, - ) -> BOOL; pub fn CreateNamedPipeW( lpName: LPCWSTR, dwOpenMode: DWORD, diff --git a/library/std/src/sys/windows/handle.rs b/library/std/src/sys/windows/handle.rs index c319cb28630f5..ef9a8bd690031 100644 --- a/library/std/src/sys/windows/handle.rs +++ b/library/std/src/sys/windows/handle.rs @@ -221,11 +221,6 @@ impl Handle { Ok(Self(self.0.duplicate(access, inherit, options)?)) } - #[cfg(not(target_vendor = "uwp"))] - pub(crate) fn set_inheritable(&self) -> io::Result<()> { - self.0.set_inheritable() - } - /// Performs a synchronous read. /// /// If the handle is opened for asynchronous I/O then this abort the process. diff --git a/library/std/src/sys/windows/pipe.rs b/library/std/src/sys/windows/pipe.rs index 2c586f1abe433..013c776c476c3 100644 --- a/library/std/src/sys/windows/pipe.rs +++ b/library/std/src/sys/windows/pipe.rs @@ -18,20 +18,13 @@ use crate::sys_common::IntoInner; // Anonymous pipes //////////////////////////////////////////////////////////////////////////////// -// A 64kb pipe capacity is the same as a typical Linux default. -const PIPE_BUFFER_CAPACITY: u32 = 64 * 1024; - -pub enum AnonPipe { - Sync(Handle), - Async(Handle), +pub struct AnonPipe { + inner: Handle, } impl IntoInner for AnonPipe { fn into_inner(self) -> Handle { - match self { - Self::Sync(handle) => handle, - Self::Async(handle) => handle, - } + self.inner } } @@ -39,46 +32,6 @@ pub struct Pipes { pub ours: AnonPipe, pub theirs: AnonPipe, } -impl Pipes { - /// Create a new pair of pipes where both pipes are synchronous. - /// - /// These must not be used asynchronously. - pub fn new_synchronous( - ours_readable: bool, - their_handle_inheritable: bool, - ) -> io::Result { - unsafe { - // If `CreatePipe` succeeds, these will be our pipes. - let mut read = ptr::null_mut(); - let mut write = ptr::null_mut(); - - if c::CreatePipe(&mut read, &mut write, ptr::null(), PIPE_BUFFER_CAPACITY) == 0 { - Err(io::Error::last_os_error()) - } else { - let (ours, theirs) = if ours_readable { (read, write) } else { (write, read) }; - let ours = Handle::from_raw_handle(ours); - #[cfg(not(target_vendor = "uwp"))] - let theirs = Handle::from_raw_handle(theirs); - #[cfg(target_vendor = "uwp")] - let mut theirs = Handle::from_raw_handle(theirs); - - if their_handle_inheritable { - #[cfg(not(target_vendor = "uwp"))] - { - theirs.set_inheritable()?; - } - - #[cfg(target_vendor = "uwp")] - { - theirs = theirs.duplicate(0, true, c::DUPLICATE_SAME_ACCESS)?; - } - } - - Ok(Pipes { ours: AnonPipe::Sync(ours), theirs: AnonPipe::Sync(theirs) }) - } - } - } -} /// 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, @@ -100,6 +53,9 @@ impl Pipes { /// 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 { + // A 64kb pipe capacity is the same as a typical Linux default. + const PIPE_BUFFER_CAPACITY: u32 = 64 * 1024; + // 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 @@ -200,9 +156,12 @@ pub fn anon_pipe(ours_readable: bool, their_handle_inheritable: bool) -> io::Res }; opts.security_attributes(&mut sa); let theirs = File::open(Path::new(&name), &opts)?; - let theirs = AnonPipe::Sync(theirs.into_inner()); + let theirs = AnonPipe { inner: theirs.into_inner() }; - Ok(Pipes { ours: AnonPipe::Async(ours), theirs }) + Ok(Pipes { + ours: AnonPipe { inner: ours }, + theirs: AnonPipe { inner: theirs.into_inner() }, + }) } } @@ -212,12 +171,12 @@ pub fn anon_pipe(ours_readable: bool, their_handle_inheritable: bool) -> io::Res /// This is achieved by creating a new set of pipes and spawning a thread that /// relays messages between the source and the synchronous pipe. pub fn spawn_pipe_relay( - source: &Handle, + source: &AnonPipe, ours_readable: bool, their_handle_inheritable: bool, ) -> io::Result { // We need this handle to live for the lifetime of the thread spawned below. - let source = AnonPipe::Async(source.duplicate(0, true, c::DUPLICATE_SAME_ACCESS)?); + let source = source.duplicate()?; // create a new pair of anon pipes. let Pipes { theirs, ours } = anon_pipe(ours_readable, their_handle_inheritable)?; @@ -268,24 +227,19 @@ type AlertableIoFn = unsafe extern "system" fn( impl AnonPipe { pub fn handle(&self) -> &Handle { - match self { - Self::Async(ref handle) => handle, - Self::Sync(ref handle) => handle, - } + &self.inner } pub fn into_handle(self) -> Handle { - self.into_inner() + self.inner + } + fn duplicate(&self) -> io::Result { + self.inner.duplicate(0, false, c::DUPLICATE_SAME_ACCESS).map(|inner| AnonPipe { inner }) } pub fn read(&self, buf: &mut [u8]) -> io::Result { let result = unsafe { let len = crate::cmp::min(buf.len(), c::DWORD::MAX as usize) as c::DWORD; - match self { - Self::Sync(ref handle) => handle.read(buf), - Self::Async(_) => { - self.alertable_io_internal(c::ReadFileEx, buf.as_mut_ptr() as _, len) - } - } + self.alertable_io_internal(c::ReadFileEx, buf.as_mut_ptr() as _, len) }; match result { @@ -299,33 +253,28 @@ impl AnonPipe { } pub fn read_vectored(&self, bufs: &mut [IoSliceMut<'_>]) -> io::Result { - io::default_read_vectored(|buf| self.read(buf), bufs) + self.inner.read_vectored(bufs) } #[inline] pub fn is_read_vectored(&self) -> bool { - false + self.inner.is_read_vectored() } pub fn write(&self, buf: &[u8]) -> io::Result { unsafe { let len = crate::cmp::min(buf.len(), c::DWORD::MAX as usize) as c::DWORD; - match self { - Self::Sync(ref handle) => handle.write(buf), - Self::Async(_) => { - self.alertable_io_internal(c::WriteFileEx, buf.as_ptr() as _, len) - } - } + self.alertable_io_internal(c::WriteFileEx, buf.as_ptr() as _, len) } } pub fn write_vectored(&self, bufs: &[IoSlice<'_>]) -> io::Result { - io::default_write_vectored(|buf| self.write(buf), bufs) + self.inner.write_vectored(bufs) } #[inline] pub fn is_write_vectored(&self) -> bool { - false + self.inner.is_write_vectored() } /// Synchronizes asynchronous reads or writes using our anonymous pipe. @@ -397,7 +346,7 @@ impl AnonPipe { // Asynchronous read of the pipe. // If successful, `callback` will be called once it completes. - let result = io(self.handle().as_handle(), buf, len, &mut overlapped, callback); + let result = io(self.inner.as_handle(), buf, len, &mut overlapped, callback); if result == c::FALSE { // We can return here because the call failed. // After this we must not return until the I/O completes. diff --git a/library/std/src/sys/windows/process.rs b/library/std/src/sys/windows/process.rs index 8e5325b80e4a5..9fd399f4ba1d3 100644 --- a/library/std/src/sys/windows/process.rs +++ b/library/std/src/sys/windows/process.rs @@ -23,7 +23,7 @@ use crate::sys::cvt; use crate::sys::fs::{File, OpenOptions}; use crate::sys::handle::Handle; use crate::sys::path; -use crate::sys::pipe::{self, AnonPipe, Pipes}; +use crate::sys::pipe::{self, AnonPipe}; use crate::sys::stdio; use crate::sys_common::mutex::StaticMutex; use crate::sys_common::process::{CommandEnv, CommandEnvs}; @@ -172,7 +172,7 @@ pub enum Stdio { Inherit, Null, MakePipe, - AsyncPipe(Handle), + Pipe(AnonPipe), Handle(Handle), } @@ -527,33 +527,13 @@ impl Stdio { }, Stdio::MakePipe => { - // Handles that are passed to a child process must be synchronous - // because they will be read synchronously (see #95759). - // Therefore we prefer to make both ends of a pipe synchronous - // just in case our end of the pipe is passed to another process. - // - // However, we may need to read from both the child's stdout and - // stderr simultaneously when waiting for output. This requires - // async reads so as to avoid blocking either pipe. - // - // The solution used here is to make handles synchronous - // except for our side of the stdout and sterr pipes. - // If our side of those pipes do end up being given to another - // process then we use a "pipe relay" to synchronize access - // (see `Stdio::AsyncPipe` below). - let pipes = if stdio_id == c::STD_INPUT_HANDLE { - // For stdin both sides of the pipe are synchronous. - Pipes::new_synchronous(false, true)? - } else { - // For stdout/stderr our side of the pipe is async and their side is synchronous. - pipe::anon_pipe(true, true)? - }; + let ours_readable = stdio_id != c::STD_INPUT_HANDLE; + let pipes = pipe::anon_pipe(ours_readable, true)?; *pipe = Some(pipes.ours); Ok(pipes.theirs.into_handle()) } - Stdio::AsyncPipe(ref source) => { - // We need to synchronize asynchronous pipes by using a pipe relay. + Stdio::Pipe(ref source) => { let ours_readable = stdio_id != c::STD_INPUT_HANDLE; pipe::spawn_pipe_relay(source, ours_readable, true).map(AnonPipe::into_handle) } @@ -582,13 +562,7 @@ impl Stdio { impl From for Stdio { fn from(pipe: AnonPipe) -> Stdio { - // Note that it's very important we don't give async handles to child processes. - // Therefore if the pipe is asynchronous we must have a way to turn it synchronous. - // See #95759. - match pipe { - AnonPipe::Sync(handle) => Stdio::Handle(handle), - AnonPipe::Async(handle) => Stdio::AsyncPipe(handle), - } + Stdio::Pipe(pipe) } }