Auto merge of #95469 - ChrisDenton:unsound-read-write, r=joshtriplett

Fix unsound `File` methods

This is a draft attempt to fix #81357. *EDIT*: this PR now tackles `read()`, `write()`, `read_at()`, `write_at()` and `read_buf`. Still needs more testing though.

cc `@jstarks,` can you confirm the the Windows team is ok with the Rust stdlib using `NtReadFile` and `NtWriteFile`?

~Also, I'm provisionally using `CancelIo` in a last ditch attempt to recover but I'm not sure that this is actually a good idea. Especially as getting into this state would be a programmer error so aborting the process is justified in any case.~ *EDIT*: removed, see comments.

Author: bors

library/std/src/fs.rs

@@ -85,6 +85,12 @@ use crate::time::SystemTime;
 /// by different processes. Avoid assuming that holding a `&File` means that the
 /// file will not change.
 ///
+/// # Platform-specific behavior
+///
+/// On Windows, the implementation of [`Read`] and [`Write`] traits for `File`
+/// perform synchronous I/O operations. Therefore the underlying file must not
+/// have been opened for asynchronous I/O (e.g. by using `FILE_FLAG_OVERLAPPED`).
+///
 /// [`BufReader<R>`]: io::BufReader
 /// [`sync_all`]: File::sync_all
 #[stable(feature = "rust1", since = "1.0.0")]

library/std/src/sys/windows/c.rs

@@ -274,6 +274,9 @@ pub const STATUS_SUCCESS: NTSTATUS = 0x00000000;
 pub const STATUS_DELETE_PENDING: NTSTATUS = 0xc0000056_u32 as _;
 pub const STATUS_INVALID_PARAMETER: NTSTATUS = 0xc000000d_u32 as _;
 
+pub const STATUS_PENDING: NTSTATUS = 0x103 as _;
+pub const STATUS_END_OF_FILE: NTSTATUS = 0xC0000011_u32 as _;
+
 // Equivalent to the `NT_SUCCESS` C preprocessor macro.
 // See: https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/using-ntstatus-values
 pub fn nt_success(status: NTSTATUS) -> bool {
@@ -316,22 +319,34 @@ impl Default for OBJECT_ATTRIBUTES {
     }
 }
 #[repr(C)]
-pub struct IO_STATUS_BLOCK {
-    pub Pointer: *mut c_void,
-    pub Information: usize,
+union IO_STATUS_BLOCK_union {
+    Status: NTSTATUS,
+    Pointer: *mut c_void,
 }
-impl Default for IO_STATUS_BLOCK {
+impl Default for IO_STATUS_BLOCK_union {
     fn default() -> Self {
-        Self { Pointer: ptr::null_mut(), Information: 0 }
+        Self { Pointer: ptr::null_mut() }
     }
 }
+#[repr(C)]
+#[derive(Default)]
+pub struct IO_STATUS_BLOCK {
+    u: IO_STATUS_BLOCK_union,
+    pub Information: usize,
+}
 
 pub type LPOVERLAPPED_COMPLETION_ROUTINE = unsafe extern "system" fn(
     dwErrorCode: DWORD,
     dwNumberOfBytesTransfered: DWORD,
     lpOverlapped: *mut OVERLAPPED,
 );
 
+type IO_APC_ROUTINE = unsafe extern "system" fn(
+    ApcContext: *mut c_void,
+    IoStatusBlock: *mut IO_STATUS_BLOCK,
+    Reserved: ULONG,
+);
+
 #[repr(C)]
 #[cfg(not(target_pointer_width = "64"))]
 pub struct WSADATA {
@@ -971,13 +986,6 @@ extern "system" {
         lpOverlapped: LPOVERLAPPED,
         lpCompletionRoutine: LPOVERLAPPED_COMPLETION_ROUTINE,
     ) -> BOOL;
-    pub fn WriteFile(
-        hFile: BorrowedHandle<'_>,
-        lpBuffer: LPVOID,
-        nNumberOfBytesToWrite: DWORD,
-        lpNumberOfBytesWritten: LPDWORD,
-        lpOverlapped: LPOVERLAPPED,
-    ) -> BOOL;
     pub fn WriteFileEx(
         hFile: BorrowedHandle<'_>,
         lpBuffer: LPVOID,
@@ -1268,6 +1276,32 @@ compat_fn! {
     ) -> NTSTATUS {
         panic!("`NtCreateFile` not available");
     }
+    pub fn NtReadFile(
+        FileHandle: BorrowedHandle<'_>,
+        Event: HANDLE,
+        ApcRoutine: Option<IO_APC_ROUTINE>,
+        ApcContext: *mut c_void,
+        IoStatusBlock: &mut IO_STATUS_BLOCK,
+        Buffer: *mut crate::mem::MaybeUninit<u8>,
+        Length: ULONG,
+        ByteOffset: Option<&LARGE_INTEGER>,
+        Key: Option<&ULONG>
+    ) -> NTSTATUS {
+        panic!("`NtReadFile` not available");
+    }
+    pub fn NtWriteFile(
+        FileHandle: BorrowedHandle<'_>,
+        Event: HANDLE,
+        ApcRoutine: Option<IO_APC_ROUTINE>,
+        ApcContext: *mut c_void,
+        IoStatusBlock: &mut IO_STATUS_BLOCK,
+        Buffer: *const u8,
+        Length: ULONG,
+        ByteOffset: Option<&LARGE_INTEGER>,
+        Key: Option<&ULONG>
+    ) -> NTSTATUS {
+        panic!("`NtWriteFile` not available");
+    }
     pub fn RtlNtStatusToDosError(
         Status: NTSTATUS
     ) -> ULONG {

library/std/src/sys/windows/handle.rs

@@ -74,20 +74,10 @@ impl FromRawHandle for Handle {
 
 impl Handle {
     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(),
-            )
-        });
+        let res = unsafe { self.synchronous_read(buf.as_mut_ptr().cast(), buf.len(), None) };
 
         match res {
-            Ok(_) => Ok(read as usize),
+            Ok(read) => Ok(read as usize),
 
             // The special treatment of BrokenPipe is to deal with Windows
             // pipe semantics, which yields this error when *reading* from
@@ -109,42 +99,23 @@ impl Handle {
     }
 
     pub fn read_at(&self, buf: &mut [u8], offset: u64) -> io::Result<usize> {
-        let mut read = 0;
-        let len = cmp::min(buf.len(), <c::DWORD>::MAX as usize) as c::DWORD;
-        let res = unsafe {
-            let mut overlapped: c::OVERLAPPED = mem::zeroed();
-            overlapped.Offset = offset as u32;
-            overlapped.OffsetHigh = (offset >> 32) as u32;
-            cvt(c::ReadFile(
-                self.as_handle(),
-                buf.as_mut_ptr() as c::LPVOID,
-                len,
-                &mut read,
-                &mut overlapped,
-            ))
-        };
+        let res =
+            unsafe { self.synchronous_read(buf.as_mut_ptr().cast(), buf.len(), Some(offset)) };
+
         match res {
-            Ok(_) => Ok(read as usize),
+            Ok(read) => Ok(read as usize),
             Err(ref e) if e.raw_os_error() == Some(c::ERROR_HANDLE_EOF as i32) => Ok(0),
             Err(e) => Err(e),
         }
     }
 
     pub fn read_buf(&self, buf: &mut ReadBuf<'_>) -> io::Result<()> {
-        let mut read = 0;
-        let len = cmp::min(buf.remaining(), <c::DWORD>::MAX as usize) as c::DWORD;
-        let res = cvt(unsafe {
-            c::ReadFile(
-                self.as_handle(),
-                buf.unfilled_mut().as_mut_ptr() as c::LPVOID,
-                len,
-                &mut read,
-                ptr::null_mut(),
-            )
-        });
+        let res = unsafe {
+            self.synchronous_read(buf.unfilled_mut().as_mut_ptr(), buf.remaining(), None)
+        };
 
         match res {
-            Ok(_) => {
+            Ok(read) => {
                 // Safety: `read` bytes were written to the initialized portion of the buffer
                 unsafe {
                     buf.assume_init(read as usize);
@@ -221,18 +192,7 @@ impl Handle {
     }
 
     pub fn write(&self, buf: &[u8]) -> io::Result<usize> {
-        let mut amt = 0;
-        let len = cmp::min(buf.len(), <c::DWORD>::MAX as usize) as c::DWORD;
-        cvt(unsafe {
-            c::WriteFile(
-                self.as_handle(),
-                buf.as_ptr() as c::LPVOID,
-                len,
-                &mut amt,
-                ptr::null_mut(),
-            )
-        })?;
-        Ok(amt as usize)
+        self.synchronous_write(&buf, None)
     }
 
     pub fn write_vectored(&self, bufs: &[IoSlice<'_>]) -> io::Result<usize> {
@@ -245,21 +205,7 @@ impl Handle {
     }
 
     pub fn write_at(&self, buf: &[u8], offset: u64) -> io::Result<usize> {
-        let mut written = 0;
-        let len = cmp::min(buf.len(), <c::DWORD>::MAX as usize) as c::DWORD;
-        unsafe {
-            let mut overlapped: c::OVERLAPPED = mem::zeroed();
-            overlapped.Offset = offset as u32;
-            overlapped.OffsetHigh = (offset >> 32) as u32;
-            cvt(c::WriteFile(
-                self.as_handle(),
-                buf.as_ptr() as c::LPVOID,
-                len,
-                &mut written,
-                &mut overlapped,
-            ))?;
-        }
-        Ok(written as usize)
+        self.synchronous_write(&buf, Some(offset))
     }
 
     pub fn try_clone(&self) -> io::Result<Self> {
@@ -274,6 +220,97 @@ impl Handle {
     ) -> io::Result<Self> {
         Ok(Self(self.0.duplicate(access, inherit, options)?))
     }
+
+    /// Performs a synchronous read.
+    ///
+    /// If the handle is opened for asynchronous I/O then this abort the process.
+    /// See #81357.
+    ///
+    /// If `offset` is `None` then the current file position is used.
+    unsafe fn synchronous_read(
+        &self,
+        buf: *mut mem::MaybeUninit<u8>,
+        len: usize,
+        offset: Option<u64>,
+    ) -> io::Result<usize> {
+        let mut io_status = c::IO_STATUS_BLOCK::default();
+
+        // The length is clamped at u32::MAX.
+        let len = cmp::min(len, c::DWORD::MAX as usize) as c::DWORD;
+        let status = c::NtReadFile(
+            self.as_handle(),
+            ptr::null_mut(),
+            None,
+            ptr::null_mut(),
+            &mut io_status,
+            buf,
+            len,
+            offset.map(|n| n as _).as_ref(),
+            None,
+        );
+        match status {
+            // 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();
+            }
+
+            // Return `Ok(0)` when there's nothing more to read.
+            c::STATUS_END_OF_FILE => Ok(0),
+
+            // Success!
+            status if c::nt_success(status) => Ok(io_status.Information),
+
+            status => {
+                let error = c::RtlNtStatusToDosError(status);
+                Err(io::Error::from_raw_os_error(error as _))
+            }
+        }
+    }
+
+    /// Performs a synchronous write.
+    ///
+    /// If the handle is opened for asynchronous I/O then this abort the process.
+    /// See #81357.
+    ///
+    /// If `offset` is `None` then the current file position is used.
+    fn synchronous_write(&self, buf: &[u8], offset: Option<u64>) -> io::Result<usize> {
+        let mut io_status = c::IO_STATUS_BLOCK::default();
+
+        // The length is clamped at u32::MAX.
+        let len = cmp::min(buf.len(), c::DWORD::MAX as usize) as c::DWORD;
+        let status = unsafe {
+            c::NtWriteFile(
+                self.as_handle(),
+                ptr::null_mut(),
+                None,
+                ptr::null_mut(),
+                &mut io_status,
+                buf.as_ptr(),
+                len,
+                offset.map(|n| n as _).as_ref(),
+                None,
+            )
+        };
+        match status {
+            // 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");
+            }
+
+            // Success!
+            status if c::nt_success(status) => Ok(io_status.Information),
+
+            status => {
+                let error = unsafe { c::RtlNtStatusToDosError(status) };
+                Err(io::Error::from_raw_os_error(error as _))
+            }
+        }
+    }
 }
 
 impl<'a> Read for &'a Handle {