Fix some possible UB in std::sys::windows

thomcc · thomcc · commit 1b8025a24c4b · 2022-08-29T15:59:34.000-07:00
diff --git a/library/std/src/sys/windows/c.rs b/library/std/src/sys/windows/c.rs
@@ -503,6 +503,8 @@ pub struct FILE_END_OF_FILE_INFO {
     pub EndOfFile: LARGE_INTEGER,
 }
 
+/// NB: Use carefully! In general using this as a reference is likely to get the
+/// provenance wrong for the `rest` field!
 #[repr(C)]
 pub struct REPARSE_DATA_BUFFER {
     pub ReparseTag: c_uint,
@@ -511,6 +513,8 @@ pub struct REPARSE_DATA_BUFFER {
     pub rest: (),
 }
 
+/// NB: Use carefully! In general using this as a reference is likely to get the
+/// provenance wrong for the `PathBuffer` field!
 #[repr(C)]
 pub struct SYMBOLIC_LINK_REPARSE_BUFFER {
     pub SubstituteNameOffset: c_ushort,
@@ -521,6 +525,8 @@ pub struct SYMBOLIC_LINK_REPARSE_BUFFER {
     pub PathBuffer: WCHAR,
 }
 
+/// NB: Use carefully! In general using this as a reference is likely to get the
+/// provenance wrong for the `PathBuffer` field!
 #[repr(C)]
 pub struct MOUNT_POINT_REPARSE_BUFFER {
     pub SubstituteNameOffset: c_ushort,
diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs
@@ -11,7 +11,7 @@ use crate::slice;
 use crate::sync::Arc;
 use crate::sys::handle::Handle;
 use crate::sys::time::SystemTime;
-use crate::sys::{c, cvt};
+use crate::sys::{c, cvt, AlignedAs};
 use crate::sys_common::{AsInner, FromInner, IntoInner};
 use crate::thread;
 
@@ -47,6 +47,9 @@ pub struct ReadDir {
     first: Option<c::WIN32_FIND_DATAW>,
 }
 
+type AlignedReparseBuf =
+    AlignedAs<c::REPARSE_DATA_BUFFER, [u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]>;
+
 struct FindNextFileHandle(c::HANDLE);
 
 unsafe impl Send for FindNextFileHandle {}
@@ -326,9 +329,9 @@ impl File {
             cvt(c::GetFileInformationByHandle(self.handle.as_raw_handle(), &mut info))?;
             let mut reparse_tag = 0;
             if info.dwFileAttributes & c::FILE_ATTRIBUTE_REPARSE_POINT != 0 {
-                let mut b = [0; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE];
+                let mut b = AlignedReparseBuf::new([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]);
                 if let Ok((_, buf)) = self.reparse_point(&mut b) {
-                    reparse_tag = buf.ReparseTag;
+                    reparse_tag = (*buf).ReparseTag;
                 }
             }
             Ok(FileAttr {
@@ -389,7 +392,7 @@ impl File {
             attr.file_size = info.AllocationSize as u64;
             attr.number_of_links = Some(info.NumberOfLinks);
             if attr.file_type().is_reparse_point() {
-                let mut b = [0; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE];
+                let mut b = AlignedReparseBuf::new([0; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]);
                 if let Ok((_, buf)) = self.reparse_point(&mut b) {
                     attr.reparse_tag = buf.ReparseTag;
                 }
@@ -458,10 +461,13 @@ impl File {
         Ok(Self { handle: self.handle.try_clone()? })
     }
 
-    fn reparse_point<'a>(
+    // NB: returned pointer is derived from `space`, and has provenance to
+    // match. A raw pointer is returned rather than a reference in order to
+    // avoid narrowing provenance to the actual `REPARSE_DATA_BUFFER`.
+    fn reparse_point(
         &self,
-        space: &'a mut [u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE],
-    ) -> io::Result<(c::DWORD, &'a c::REPARSE_DATA_BUFFER)> {
+        space: &mut AlignedReparseBuf,
+    ) -> io::Result<(c::DWORD, *const c::REPARSE_DATA_BUFFER)> {
         unsafe {
             let mut bytes = 0;
             cvt({
@@ -470,36 +476,38 @@ impl File {
                     c::FSCTL_GET_REPARSE_POINT,
                     ptr::null_mut(),
                     0,
-                    space.as_mut_ptr() as *mut _,
-                    space.len() as c::DWORD,
+                    space.value.as_mut_ptr() as *mut _,
+                    space.value.len() as c::DWORD,
                     &mut bytes,
                     ptr::null_mut(),
                 )
             })?;
-            Ok((bytes, &*(space.as_ptr() as *const c::REPARSE_DATA_BUFFER)))
+            Ok((bytes, space.value.as_ptr().cast::<c::REPARSE_DATA_BUFFER>()))
         }
     }
 
     fn readlink(&self) -> io::Result<PathBuf> {
-        let mut space = [0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE];
+        let mut space = AlignedReparseBuf::new([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]);
         let (_bytes, buf) = self.reparse_point(&mut space)?;
         unsafe {
-            let (path_buffer, subst_off, subst_len, relative) = match buf.ReparseTag {
+            let (path_buffer, subst_off, subst_len, relative) = match (*buf).ReparseTag {
                 c::IO_REPARSE_TAG_SYMLINK => {
                     let info: *const c::SYMBOLIC_LINK_REPARSE_BUFFER =
-                        &buf.rest as *const _ as *const _;
+                        ptr::addr_of!((*buf).rest).cast();
+                    assert!(info.is_aligned());
                     (
-                        &(*info).PathBuffer as *const _ as *const u16,
+                        ptr::addr_of!((*info).PathBuffer).cast::<u16>(),
                         (*info).SubstituteNameOffset / 2,
                         (*info).SubstituteNameLength / 2,
                         (*info).Flags & c::SYMLINK_FLAG_RELATIVE != 0,
                     )
                 }
                 c::IO_REPARSE_TAG_MOUNT_POINT => {
                     let info: *const c::MOUNT_POINT_REPARSE_BUFFER =
-                        &buf.rest as *const _ as *const _;
+                        ptr::addr_of!((*buf).rest).cast();
+                    assert!(info.is_aligned());
                     (
-                        &(*info).PathBuffer as *const _ as *const u16,
+                        ptr::addr_of!((*info).PathBuffer).cast::<u16>(),
                         (*info).SubstituteNameOffset / 2,
                         (*info).SubstituteNameLength / 2,
                         false,
diff --git a/library/std/src/sys/windows/mod.rs b/library/std/src/sys/windows/mod.rs
@@ -329,3 +329,26 @@ pub fn abort_internal() -> ! {
     }
     crate::intrinsics::abort();
 }
+
+/// Used for some win32 buffers which are stack allocated, for example:
+/// `AlignedAs<c::REPARSE_DATA_BUFFER, [u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]>`
+#[repr(C)]
+#[derive(Copy, Clone)]
+pub struct AlignedAs<Aligner, Alignee: ?Sized> {
+    /// Use `[Aligner; 0]` as a sort of `PhantomAlignNextField<Aligner>`. This
+    /// is a bit of a hack, and could break (in a way that's caught by tests) if
+    /// #81996 is fixed.
+    aligner: [Aligner; 0],
+    /// The aligned value. Public rather than exposed via accessors so that if
+    /// needed it can be used with `addr_of` and such (also, this is less code).
+    pub value: Alignee,
+}
+
+impl<Aligner, Alignee> AlignedAs<Aligner, Alignee> {
+    // This is frequently used with large stack buffers, so force-inline to
+    // try and avoid using 2x as much stack space in debug builds.
+    #[inline(always)]
+    pub const fn new(value: Alignee) -> Self {
+        Self { aligner: [], value }
+    }
+}
diff --git a/library/std/src/sys/windows/os/tests.rs b/library/std/src/sys/windows/os/tests.rs
@@ -11,3 +11,21 @@ fn ntstatus_error() {
             .contains("FormatMessageW() returned error")
     );
 }
+
+#[test]
+fn smoketest_aligned_as() {
+    use crate::{
+        mem::{align_of, size_of},
+        ptr::addr_of,
+        sys::{c, AlignedAs},
+    };
+    type AlignedReparseBuf =
+        AlignedAs<c::REPARSE_DATA_BUFFER, [u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]>;
+    assert!(size_of::<AlignedReparseBuf>() >= c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE);
+    assert_eq!(align_of::<AlignedReparseBuf>(), align_of::<c::REPARSE_DATA_BUFFER>());
+    let a = AlignedReparseBuf::new([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]);
+    // Quick and dirty offsetof check.
+    assert_eq!(addr_of!(a).cast::<u8>(), addr_of!(a.value).cast::<u8>());
+    // Smoke check that it's actually aligned.
+    assert!(addr_of!(a.value).is_aligned_to(align_of::<c::REPARSE_DATA_BUFFER>()));
+}
