From ae60dbdcac2c4c94293980cdb6872228fc4d8aa9 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Wed, 6 Jul 2022 16:36:52 +0100 Subject: [PATCH 1/4] Use `rtabort!` instead of `process::abort` --- library/std/src/sys/windows/handle.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/library/std/src/sys/windows/handle.rs b/library/std/src/sys/windows/handle.rs index 1e7b6e1eab03a..cea04549d3d90 100644 --- a/library/std/src/sys/windows/handle.rs +++ b/library/std/src/sys/windows/handle.rs @@ -252,10 +252,7 @@ impl Handle { // If the operation has not completed then abort the process. // Doing otherwise means that the buffer and stack may be written to // after this function returns. - c::STATUS_PENDING => { - eprintln!("I/O error: operation failed to complete synchronously"); - crate::process::abort(); - } + c::STATUS_PENDING => rtabort!("I/O error: operation failed to complete synchronously"), // Return `Ok(0)` when there's nothing more to read. c::STATUS_END_OF_FILE => Ok(0), @@ -298,9 +295,7 @@ impl Handle { // If the operation has not completed then abort the process. // Doing otherwise means that the buffer may be read and the stack // written to after this function returns. - c::STATUS_PENDING => { - rtabort!("I/O error: operation failed to complete synchronously"); - } + c::STATUS_PENDING => rtabort!("I/O error: operation failed to complete synchronously"), // Success! status if c::nt_success(status) => Ok(io_status.Information), From 3ae47e76a84baaa4d35f641c237649944fa8fb58 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Wed, 6 Jul 2022 16:38:35 +0100 Subject: [PATCH 2/4] Windows: Fallback for overlapped I/O Try waiting on the file handle once. If that fails then give up. --- library/std/src/sys/windows/c.rs | 14 +++++++++++++- library/std/src/sys/windows/handle.rs | 13 +++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/library/std/src/sys/windows/c.rs b/library/std/src/sys/windows/c.rs index 5469487df1eed..c7a42ef9a9382 100644 --- a/library/std/src/sys/windows/c.rs +++ b/library/std/src/sys/windows/c.rs @@ -326,7 +326,9 @@ union IO_STATUS_BLOCK_union { } impl Default for IO_STATUS_BLOCK_union { fn default() -> Self { - Self { Pointer: ptr::null_mut() } + let mut this = Self { Pointer: ptr::null_mut() }; + this.Status = STATUS_PENDING; + this } } #[repr(C)] @@ -335,6 +337,16 @@ pub struct IO_STATUS_BLOCK { u: IO_STATUS_BLOCK_union, pub Information: usize, } +impl IO_STATUS_BLOCK { + pub fn status(&self) -> NTSTATUS { + // SAFETY: If `self.u.Status` was set then this is obviously safe. + // If `self.u.Pointer` was set then this is the equivalent to converting + // the pointer to an integer, which is also safe. + // Currently the only safe way to construct `IO_STATUS_BLOCK` outside of + // this module is to call the `default` method, which sets the `Status`. + unsafe { self.u.Status } + } +} pub type LPOVERLAPPED_COMPLETION_ROUTINE = unsafe extern "system" fn( dwErrorCode: DWORD, diff --git a/library/std/src/sys/windows/handle.rs b/library/std/src/sys/windows/handle.rs index cea04549d3d90..6dd022b0806ba 100644 --- a/library/std/src/sys/windows/handle.rs +++ b/library/std/src/sys/windows/handle.rs @@ -248,6 +248,13 @@ impl Handle { offset.map(|n| n as _).as_ref(), None, ); + + let status = if status == c::STATUS_PENDING { + c::WaitForSingleObject(self.as_raw_handle(), c::INFINITE); + io_status.status() + } else { + status + }; match status { // If the operation has not completed then abort the process. // Doing otherwise means that the buffer and stack may be written to @@ -291,6 +298,12 @@ impl Handle { None, ) }; + let status = if status == c::STATUS_PENDING { + unsafe { c::WaitForSingleObject(self.as_raw_handle(), c::INFINITE) }; + io_status.status() + } else { + status + }; match status { // If the operation has not completed then abort the process. // Doing otherwise means that the buffer may be read and the stack From a8ffc7fd45592fd423d44852383efb3c6c1a7264 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Wed, 6 Jul 2022 16:39:06 +0100 Subject: [PATCH 3/4] Tests for unsound Windows file methods --- library/std/src/sys/windows/handle.rs | 3 + library/std/src/sys/windows/handle/tests.rs | 22 ++++++ .../issues-81357-unsound-file-methods.rs | 76 +++++++++++++++++++ 3 files changed, 101 insertions(+) create mode 100644 library/std/src/sys/windows/handle/tests.rs create mode 100644 src/test/ui-fulldeps/issues-81357-unsound-file-methods.rs diff --git a/library/std/src/sys/windows/handle.rs b/library/std/src/sys/windows/handle.rs index 6dd022b0806ba..e24b09cc96ec8 100644 --- a/library/std/src/sys/windows/handle.rs +++ b/library/std/src/sys/windows/handle.rs @@ -1,5 +1,8 @@ #![unstable(issue = "none", feature = "windows_handle")] +#[cfg(test)] +mod tests; + use crate::cmp; use crate::io::{self, ErrorKind, IoSlice, IoSliceMut, Read, ReadBuf}; use crate::mem; diff --git a/library/std/src/sys/windows/handle/tests.rs b/library/std/src/sys/windows/handle/tests.rs new file mode 100644 index 0000000000000..d836dae4c305b --- /dev/null +++ b/library/std/src/sys/windows/handle/tests.rs @@ -0,0 +1,22 @@ +use crate::sys::pipe::{anon_pipe, Pipes}; +use crate::{thread, time}; + +/// Test the synchronous fallback for overlapped I/O. +#[test] +fn overlapped_handle_fallback() { + // Create some pipes. `ours` will be asynchronous. + let Pipes { ours, theirs } = anon_pipe(true, false).unwrap(); + + let async_readable = ours.into_handle(); + let sync_writeable = theirs.into_handle(); + + thread::scope(|_| { + thread::sleep(time::Duration::from_millis(100)); + sync_writeable.write(b"hello world!").unwrap(); + }); + + // The pipe buffer starts empty so reading won't complete synchronously unless + // our fallback path works. + let mut buffer = [0u8; 1024]; + async_readable.read(&mut buffer).unwrap(); +} diff --git a/src/test/ui-fulldeps/issues-81357-unsound-file-methods.rs b/src/test/ui-fulldeps/issues-81357-unsound-file-methods.rs new file mode 100644 index 0000000000000..94b5798a0e7d2 --- /dev/null +++ b/src/test/ui-fulldeps/issues-81357-unsound-file-methods.rs @@ -0,0 +1,76 @@ +// run-fail +// only-windows + +fn main() { + use std::fs; + use std::io::prelude::*; + use std::os::windows::prelude::*; + use std::ptr; + use std::sync::Arc; + use std::thread; + use std::time::Duration; + + const FILE_FLAG_OVERLAPPED: u32 = 0x40000000; + + fn create_pipe_server(path: &str) -> fs::File { + let mut path0 = path.as_bytes().to_owned(); + path0.push(0); + extern "system" { + fn CreateNamedPipeA( + lpName: *const u8, + dwOpenMode: u32, + dwPipeMode: u32, + nMaxInstances: u32, + nOutBufferSize: u32, + nInBufferSize: u32, + nDefaultTimeOut: u32, + lpSecurityAttributes: *mut u8, + ) -> RawHandle; + } + + unsafe { + let h = CreateNamedPipeA(path0.as_ptr(), 3, 0, 1, 0, 0, 0, ptr::null_mut()); + assert_ne!(h as isize, -1); + fs::File::from_raw_handle(h) + } + } + + let path = "\\\\.\\pipe\\repro"; + let mut server = create_pipe_server(path); + + let client = Arc::new( + fs::OpenOptions::new().custom_flags(FILE_FLAG_OVERLAPPED).read(true).open(path).unwrap(), + ); + + let spawn_read = |is_first: bool| { + thread::spawn({ + let f = client.clone(); + move || { + let mut buf = [0xcc; 1]; + let mut f = f.as_ref(); + f.read(&mut buf).unwrap(); + if is_first { + assert_ne!(buf[0], 0xcc); + } else { + let b = buf[0]; // capture buf[0] + thread::sleep(Duration::from_millis(200)); + + // In this test, success is indicated by failing. + if buf[0] == b { + panic!("Success!"); + } + } + } + }) + }; + + let t1 = spawn_read(true); + thread::sleep(Duration::from_millis(20)); + let t2 = spawn_read(false); + thread::sleep(Duration::from_millis(100)); + let _ = server.write(b"x"); + thread::sleep(Duration::from_millis(100)); + let _ = server.write(b"y"); + let _ = t1.join(); + let _ = t2.join(); +} From 91a640176af5b164b62aa867beaf0b02fd2d93da Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Wed, 6 Jul 2022 18:47:44 +0100 Subject: [PATCH 4/4] Fix ui-fulldep test --- ...s.rs => issue-81357-unsound-file-methods.rs} | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) rename src/test/ui-fulldeps/{issues-81357-unsound-file-methods.rs => issue-81357-unsound-file-methods.rs} (83%) diff --git a/src/test/ui-fulldeps/issues-81357-unsound-file-methods.rs b/src/test/ui-fulldeps/issue-81357-unsound-file-methods.rs similarity index 83% rename from src/test/ui-fulldeps/issues-81357-unsound-file-methods.rs rename to src/test/ui-fulldeps/issue-81357-unsound-file-methods.rs index 94b5798a0e7d2..fdf1150f8d25a 100644 --- a/src/test/ui-fulldeps/issues-81357-unsound-file-methods.rs +++ b/src/test/ui-fulldeps/issue-81357-unsound-file-methods.rs @@ -55,10 +55,9 @@ fn main() { let b = buf[0]; // capture buf[0] thread::sleep(Duration::from_millis(200)); - // In this test, success is indicated by failing. - if buf[0] == b { - panic!("Success!"); - } + // Check the buffer hasn't been written to after read. + dbg!(buf[0], b); + assert_eq!(buf[0], b); } } }) @@ -71,6 +70,12 @@ fn main() { let _ = server.write(b"x"); thread::sleep(Duration::from_millis(100)); let _ = server.write(b"y"); - let _ = t1.join(); - let _ = t2.join(); + + // This is run fail because we need to test for the `abort`. + // That failing to run is the success case. + if t1.join().is_err() || t2.join().is_err() { + return; + } else { + panic!("success"); + } }