Rollup merge of #127763 - ChrisDenton:safe-unsafe-unsafe, r=tgross35

Make more Windows functions `#![deny(unsafe_op_in_unsafe_fn)]`

As part of #127747, I've evaluated some more Windows functions and added `unsafe` blocks where necessary. Some are just trivial wrappers that "inherit" the full unsafety of their function, but for others I've added some safety comments. A few functions weren't actually unsafe at all. I think they were just using `unsafe fn` to avoid an `unsafe {}` block.

I'm not touching `c.rs` yet because that is partially being addressed by another PR and also I have plans to further reduce the number of wrapper functions we have in there.

r? libs

library/std/src/sys/os_str/mod.rs

@@ -1,3 +1,5 @@
+#![forbid(unsafe_op_in_unsafe_fn)]
+
 cfg_if::cfg_if! {
     if #[cfg(any(
         target_os = "windows",

library/std/src/sys/os_str/wtf8.rs

@@ -1,6 +1,5 @@
 //! The underlying OsString/OsStr implementation on Windows is a
 //! wrapper around the "WTF-8" encoding; see the `wtf8` module for more.
-
 use crate::borrow::Cow;
 use crate::collections::TryReserveError;
 use crate::fmt;
@@ -71,7 +70,7 @@ pub fn into_encoded_bytes(self) -> Vec<u8> {
 
     #[inline]
     pub unsafe fn from_encoded_bytes_unchecked(s: Vec<u8>) -> Self {
-        Self { inner: Wtf8Buf::from_bytes_unchecked(s) }
+        unsafe { Self { inner: Wtf8Buf::from_bytes_unchecked(s) } }
     }
 
     pub fn with_capacity(capacity: usize) -> Buf {
@@ -190,7 +189,7 @@ pub fn as_encoded_bytes(&self) -> &[u8] {
 
     #[inline]
     pub unsafe fn from_encoded_bytes_unchecked(s: &[u8]) -> &Slice {
-        mem::transmute(Wtf8::from_bytes_unchecked(s))
+        unsafe { mem::transmute(Wtf8::from_bytes_unchecked(s)) }
     }
 
     #[track_caller]

library/std/src/sys/pal/windows/alloc.rs

@@ -1,5 +1,3 @@
-#![deny(unsafe_op_in_unsafe_fn)]
-
 use crate::alloc::{GlobalAlloc, Layout, System};
 use crate::ffi::c_void;
 use crate::ptr;

library/std/src/sys/pal/windows/fs.rs

@@ -1,4 +1,3 @@
-#![allow(unsafe_op_in_unsafe_fn)]
 use core::ptr::addr_of;
 
 use crate::os::windows::prelude::*;
@@ -795,10 +794,12 @@ fn next(&mut self) -> Option<Self::Item> {
 }
 
 unsafe fn from_maybe_unaligned<'a>(p: *const u16, len: usize) -> Cow<'a, [u16]> {
-    if p.is_aligned() {
-        Cow::Borrowed(crate::slice::from_raw_parts(p, len))
-    } else {
-        Cow::Owned((0..len).map(|i| p.add(i).read_unaligned()).collect())
+    unsafe {
+        if p.is_aligned() {
+            Cow::Borrowed(crate::slice::from_raw_parts(p, len))
+        } else {
+            Cow::Owned((0..len).map(|i| p.add(i).read_unaligned()).collect())
+        }
     }
 }
 
@@ -897,7 +898,9 @@ fn into_raw_handle(self) -> RawHandle {
 
 impl FromRawHandle for File {
     unsafe fn from_raw_handle(raw_handle: RawHandle) -> Self {
-        Self { handle: FromInner::from_inner(FromRawHandle::from_raw_handle(raw_handle)) }
+        unsafe {
+            Self { handle: FromInner::from_inner(FromRawHandle::from_raw_handle(raw_handle)) }
+        }
     }
 }
 
@@ -1427,10 +1430,12 @@ pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
         _hDestinationFile: c::HANDLE,
         lpData: *const c_void,
     ) -> u32 {
-        if dwStreamNumber == 1 {
-            *(lpData as *mut i64) = StreamBytesTransferred;
+        unsafe {
+            if dwStreamNumber == 1 {
+                *(lpData as *mut i64) = StreamBytesTransferred;
+            }
+            c::PROGRESS_CONTINUE
         }
-        c::PROGRESS_CONTINUE
     }
     let pfrom = maybe_verbatim(from)?;
     let pto = maybe_verbatim(to)?;

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

@@ -1,5 +1,4 @@
 #![unstable(issue = "none", feature = "windows_handle")]
-#![allow(unsafe_op_in_unsafe_fn)]
 
 #[cfg(test)]
 mod tests;
@@ -73,7 +72,7 @@ fn into_raw_handle(self) -> RawHandle {
 
 impl FromRawHandle for Handle {
     unsafe fn from_raw_handle(raw_handle: RawHandle) -> Self {
-        Self(FromRawHandle::from_raw_handle(raw_handle))
+        unsafe { Self(FromRawHandle::from_raw_handle(raw_handle)) }
     }
 }
 
@@ -139,13 +138,23 @@ pub fn read_buf(&self, mut cursor: BorrowedCursor<'_>) -> io::Result<()> {
 
     pub unsafe fn read_overlapped(
         &self,
-        buf: &mut [u8],
+        buf: &mut [mem::MaybeUninit<u8>],
         overlapped: *mut c::OVERLAPPED,
     ) -> io::Result<Option<usize>> {
-        let len = cmp::min(buf.len(), u32::MAX as usize) as u32;
-        let mut amt = 0;
-        let res =
-            cvt(c::ReadFile(self.as_raw_handle(), buf.as_mut_ptr(), len, &mut amt, overlapped));
+        // SAFETY: We have exclusive access to the buffer and it's up to the caller to
+        // ensure the OVERLAPPED pointer is valid for the lifetime of this function.
+        let (res, amt) = unsafe {
+            let len = cmp::min(buf.len(), u32::MAX as usize) as u32;
+            let mut amt = 0;
+            let res = cvt(c::ReadFile(
+                self.as_raw_handle(),
+                buf.as_mut_ptr().cast::<u8>(),
+                len,
+                &mut amt,
+                overlapped,
+            ));
+            (res, amt)
+        };
         match res {
             Ok(_) => Ok(Some(amt as usize)),
             Err(e) => {
@@ -230,20 +239,24 @@ unsafe fn synchronous_read(
 
         // The length is clamped at u32::MAX.
         let len = cmp::min(len, u32::MAX as usize) as u32;
-        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,
-        );
+        // SAFETY: It's up to the caller to ensure `buf` is writeable up to
+        // the provided `len`.
+        let status = unsafe {
+            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,
+            )
+        };
 
         let status = if status == c::STATUS_PENDING {
-            c::WaitForSingleObject(self.as_raw_handle(), c::INFINITE);
+            unsafe { c::WaitForSingleObject(self.as_raw_handle(), c::INFINITE) };
             io_status.status()
         } else {
             status
@@ -261,7 +274,7 @@ unsafe fn synchronous_read(
             status if c::nt_success(status) => Ok(io_status.Information),
 
             status => {
-                let error = c::RtlNtStatusToDosError(status);
+                let error = unsafe { c::RtlNtStatusToDosError(status) };
                 Err(io::Error::from_raw_os_error(error as _))
             }
         }

library/std/src/sys/pal/windows/io.rs

@@ -1,4 +1,3 @@
-#![allow(unsafe_op_in_unsafe_fn)]
 use crate::marker::PhantomData;
 use crate::mem::size_of;
 use crate::os::windows::io::{AsHandle, AsRawHandle, BorrowedHandle};
@@ -81,19 +80,17 @@ pub fn as_mut_slice(&mut self) -> &mut [u8] {
 }
 
 pub fn is_terminal(h: &impl AsHandle) -> bool {
-    unsafe { handle_is_console(h.as_handle()) }
+    handle_is_console(h.as_handle())
 }
 
-unsafe fn handle_is_console(handle: BorrowedHandle<'_>) -> bool {
-    let handle = handle.as_raw_handle();
-
+fn handle_is_console(handle: BorrowedHandle<'_>) -> bool {
     // A null handle means the process has no console.
-    if handle.is_null() {
+    if handle.as_raw_handle().is_null() {
         return false;
     }
 
     let mut out = 0;
-    if c::GetConsoleMode(handle, &mut out) != 0 {
+    if unsafe { c::GetConsoleMode(handle.as_raw_handle(), &mut out) != 0 } {
         // False positives aren't possible. If we got a console then we definitely have a console.
         return true;
     }
@@ -102,9 +99,9 @@ unsafe fn handle_is_console(handle: BorrowedHandle<'_>) -> bool {
     msys_tty_on(handle)
 }
 
-unsafe fn msys_tty_on(handle: c::HANDLE) -> bool {
+fn msys_tty_on(handle: BorrowedHandle<'_>) -> bool {
     // Early return if the handle is not a pipe.
-    if c::GetFileType(handle) != c::FILE_TYPE_PIPE {
+    if unsafe { c::GetFileType(handle.as_raw_handle()) != c::FILE_TYPE_PIPE } {
         return false;
     }
 
@@ -120,12 +117,14 @@ struct FILE_NAME_INFO {
     }
     let mut name_info = FILE_NAME_INFO { FileNameLength: 0, FileName: [0; c::MAX_PATH as usize] };
     // Safety: buffer length is fixed.
-    let res = c::GetFileInformationByHandleEx(
-        handle,
-        c::FileNameInfo,
-        core::ptr::addr_of_mut!(name_info) as *mut c_void,
-        size_of::<FILE_NAME_INFO>() as u32,
-    );
+    let res = unsafe {
+        c::GetFileInformationByHandleEx(
+            handle.as_raw_handle(),
+            c::FileNameInfo,
+            core::ptr::addr_of_mut!(name_info) as *mut c_void,
+            size_of::<FILE_NAME_INFO>() as u32,
+        )
+    };
     if res == 0 {
         return false;
     }

library/std/src/sys/pal/windows/os.rs

@@ -1,7 +1,6 @@
 //! Implementation of `std::os` functionality for Windows.
 
 #![allow(nonstandard_style)]
-#![allow(unsafe_op_in_unsafe_fn)]
 
 #[cfg(test)]
 mod tests;
@@ -305,15 +304,21 @@ pub fn getenv(k: &OsStr) -> Option<OsString> {
 }
 
 pub unsafe fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {
-    let k = to_u16s(k)?;
-    let v = to_u16s(v)?;
+    // SAFETY: We ensure that k and v are null-terminated wide strings.
+    unsafe {
+        let k = to_u16s(k)?;
+        let v = to_u16s(v)?;
 
-    cvt(c::SetEnvironmentVariableW(k.as_ptr(), v.as_ptr())).map(drop)
+        cvt(c::SetEnvironmentVariableW(k.as_ptr(), v.as_ptr())).map(drop)
+    }
 }
 
 pub unsafe fn unsetenv(n: &OsStr) -> io::Result<()> {
-    let v = to_u16s(n)?;
-    cvt(c::SetEnvironmentVariableW(v.as_ptr(), ptr::null())).map(drop)
+    // SAFETY: We ensure that v is a null-terminated wide strings.
+    unsafe {
+        let v = to_u16s(n)?;
+        cvt(c::SetEnvironmentVariableW(v.as_ptr(), ptr::null())).map(drop)
+    }
 }
 
 pub fn temp_dir() -> PathBuf {

library/std/src/sys/pal/windows/pipe.rs

@@ -1,4 +1,3 @@
-#![allow(unsafe_op_in_unsafe_fn)]
 use crate::os::windows::prelude::*;
 
 use crate::ffi::OsStr;
@@ -6,7 +5,6 @@
 use crate::mem;
 use crate::path::Path;
 use crate::ptr;
-use crate::slice;
 use crate::sync::atomic::AtomicUsize;
 use crate::sync::atomic::Ordering::Relaxed;
 use crate::sys::c;
@@ -325,6 +323,7 @@ pub fn is_write_vectored(&self) -> bool {
     /// [`ReadFileEx`]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfileex
     /// [`WriteFileEx`]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-writefileex
     /// [Asynchronous Procedure Call]: https://docs.microsoft.com/en-us/windows/win32/sync/asynchronous-procedure-calls
+    #[allow(unsafe_op_in_unsafe_fn)]
     unsafe fn alertable_io_internal(
         &self,
         io: AlertableIoFn,
@@ -479,8 +478,11 @@ fn new(pipe: Handle, dst: &'a mut Vec<u8>) -> io::Result<AsyncPipe<'a>> {
     fn schedule_read(&mut self) -> io::Result<bool> {
         assert_eq!(self.state, State::NotReading);
         let amt = unsafe {
-            let slice = slice_to_end(self.dst);
-            self.pipe.read_overlapped(slice, &mut *self.overlapped)?
+            if self.dst.capacity() == self.dst.len() {
+                let additional = if self.dst.capacity() == 0 { 16 } else { 1 };
+                self.dst.reserve(additional);
+            }
+            self.pipe.read_overlapped(self.dst.spare_capacity_mut(), &mut *self.overlapped)?
         };
 
         // If this read finished immediately then our overlapped event will
@@ -560,13 +562,3 @@ fn drop(&mut self) {
         }
     }
 }
-
-unsafe fn slice_to_end(v: &mut Vec<u8>) -> &mut [u8] {
-    if v.capacity() == 0 {
-        v.reserve(16);
-    }
-    if v.capacity() == v.len() {
-        v.reserve(1);
-    }
-    slice::from_raw_parts_mut(v.as_mut_ptr().add(v.len()), v.capacity() - v.len())
-}

library/std/src/sys/pal/windows/stack_overflow.rs

@@ -1,18 +1,18 @@
 #![cfg_attr(test, allow(dead_code))]
-#![allow(unsafe_op_in_unsafe_fn)]
 
 use crate::sys::c;
 use crate::thread;
 
 /// Reserve stack space for use in stack overflow exceptions.
-pub unsafe fn reserve_stack() {
-    let result = c::SetThreadStackGuarantee(&mut 0x5000);
+pub fn reserve_stack() {
+    let result = unsafe { c::SetThreadStackGuarantee(&mut 0x5000) };
     // Reserving stack space is not critical so we allow it to fail in the released build of libstd.
     // We still use debug assert here so that CI will test that we haven't made a mistake calling the function.
     debug_assert_ne!(result, 0, "failed to reserve stack space for exception handling");
 }
 
 unsafe extern "system" fn vectored_handler(ExceptionInfo: *mut c::EXCEPTION_POINTERS) -> i32 {
+    // SAFETY: It's up to the caller (which in this case is the OS) to ensure that `ExceptionInfo` is valid.
     unsafe {
         let rec = &(*(*ExceptionInfo).ExceptionRecord);
         let code = rec.ExceptionCode;
@@ -27,11 +27,14 @@ pub unsafe fn reserve_stack() {
     }
 }
 
-pub unsafe fn init() {
-    let result = c::AddVectoredExceptionHandler(0, Some(vectored_handler));
-    // Similar to the above, adding the stack overflow handler is allowed to fail
-    // but a debug assert is used so CI will still test that it normally works.
-    debug_assert!(!result.is_null(), "failed to install exception handler");
+pub fn init() {
+    // SAFETY: `vectored_handler` has the correct ABI and is safe to call during exception handling.
+    unsafe {
+        let result = c::AddVectoredExceptionHandler(0, Some(vectored_handler));
+        // Similar to the above, adding the stack overflow handler is allowed to fail
+        // but a debug assert is used so CI will still test that it normally works.
+        debug_assert!(!result.is_null(), "failed to install exception handler");
+    }
     // Set the thread stack guarantee for the main thread.
     reserve_stack();
 }

library/std/src/sys/pal/windows/thread.rs

@@ -1,4 +1,3 @@
-#![allow(unsafe_op_in_unsafe_fn)]
 use crate::ffi::CStr;
 use crate::io;
 use crate::num::NonZero;
@@ -28,21 +27,25 @@ pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
 
         // CreateThread rounds up values for the stack size to the nearest page size (at least 4kb).
         // If a value of zero is given then the default stack size is used instead.
-        let ret = c::CreateThread(
-            ptr::null_mut(),
-            stack,
-            Some(thread_start),
-            p as *mut _,
-            c::STACK_SIZE_PARAM_IS_A_RESERVATION,
-            ptr::null_mut(),
-        );
-        let ret = HandleOrNull::from_raw_handle(ret);
+        // SAFETY: `thread_start` has the right ABI for a thread's entry point.
+        // `p` is simply passed through to the new thread without being touched.
+        let ret = unsafe {
+            let ret = c::CreateThread(
+                ptr::null_mut(),
+                stack,
+                Some(thread_start),
+                p as *mut _,
+                c::STACK_SIZE_PARAM_IS_A_RESERVATION,
+                ptr::null_mut(),
+            );
+            HandleOrNull::from_raw_handle(ret)
+        };
         return if let Ok(handle) = ret.try_into() {
             Ok(Thread { handle: Handle::from_inner(handle) })
         } else {
             // The thread failed to start and as a result p was not consumed. Therefore, it is
             // safe to reconstruct the box so that it gets deallocated.
-            drop(Box::from_raw(p));
+            unsafe { drop(Box::from_raw(p)) };
             Err(io::Error::last_os_error())
         };
 
@@ -50,7 +53,9 @@ pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
             // Next, reserve some stack space for if we otherwise run out of stack.
             stack_overflow::reserve_stack();
             // Finally, let's run some code.
-            Box::from_raw(main as *mut Box<dyn FnOnce()>)();
+            // SAFETY: We are simply recreating the box that was leaked earlier.
+            // It's the responsibility of the one who call `Thread::new` to ensure this is safe to call here.
+            unsafe { Box::from_raw(main as *mut Box<dyn FnOnce()>)() };
             0
         }
     }
@@ -70,7 +75,7 @@ pub fn set_name(name: &CStr) {
     ///
     /// `name` must end with a zero value
     pub unsafe fn set_name_wide(name: &[u16]) {
-        c::SetThreadDescription(c::GetCurrentThread(), name.as_ptr());
+        unsafe { c::SetThreadDescription(c::GetCurrentThread(), name.as_ptr()) };
     }
 
     pub fn join(self) {