From 1b8025a24c4b063d2566671f0664e5dfc263c2a4 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Mon, 29 Aug 2022 15:36:01 -0700 Subject: [PATCH 1/4] Fix some possible UB in std::sys::windows --- library/std/src/sys/windows/c.rs | 6 ++++ library/std/src/sys/windows/fs.rs | 40 +++++++++++++++---------- library/std/src/sys/windows/mod.rs | 23 ++++++++++++++ library/std/src/sys/windows/os/tests.rs | 18 +++++++++++ 4 files changed, 71 insertions(+), 16 deletions(-) diff --git a/library/std/src/sys/windows/c.rs b/library/std/src/sys/windows/c.rs index ef3f6a9ba1755..865e2ad4b4347 100644 --- 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 index 9653b1abfc3d8..01f914617a073 100644 --- 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, } +type AlignedReparseBuf = + AlignedAs; + 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,26 +476,27 @@ 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::())) } } fn readlink(&self) -> io::Result { - 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::(), (*info).SubstituteNameOffset / 2, (*info).SubstituteNameLength / 2, (*info).Flags & c::SYMLINK_FLAG_RELATIVE != 0, @@ -497,9 +504,10 @@ impl File { } 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::(), (*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 index a9846a484880b..fbbc5cb79fb8b 100644 --- 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` +#[repr(C)] +#[derive(Copy, Clone)] +pub struct AlignedAs { + /// Use `[Aligner; 0]` as a sort of `PhantomAlignNextField`. 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 AlignedAs { + // 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 index 458d6e11c2098..532be0cf083ba 100644 --- 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; + assert!(size_of::() >= c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE); + assert_eq!(align_of::(), align_of::()); + let a = AlignedReparseBuf::new([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]); + // Quick and dirty offsetof check. + assert_eq!(addr_of!(a).cast::(), addr_of!(a.value).cast::()); + // Smoke check that it's actually aligned. + assert!(addr_of!(a.value).is_aligned_to(align_of::())); +} From d9c760db43d8ab701a71f633d820efc72d8cedea Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Tue, 30 Aug 2022 00:16:53 -0700 Subject: [PATCH 2/4] Fix UWP and use `AlignedReparseBuf` in `symlink_junction_inner` --- library/std/src/sys/windows/fs.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs index 01f914617a073..1545ba0d023fa 100644 --- a/library/std/src/sys/windows/fs.rs +++ b/library/std/src/sys/windows/fs.rs @@ -394,7 +394,7 @@ impl File { if attr.file_type().is_reparse_point() { 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; + attr.reparse_tag = (*buf).ReparseTag; } } Ok(attr) @@ -1345,9 +1345,10 @@ fn symlink_junction_inner(original: &Path, junction: &Path) -> io::Result<()> { let h = f.as_inner().as_raw_handle(); unsafe { - let mut data = [0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]; - let db = data.as_mut_ptr() as *mut c::REPARSE_MOUNTPOINT_DATA_BUFFER; - let buf = &mut (*db).ReparseTarget as *mut c::WCHAR; + let mut data = AlignedReparseBuf::new([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]); + let data_ptr = data.value.as_mut_ptr(); + let db = data_ptr.cast::(); + let buf = ptr::addr_of_mut!((*db).ReparseTarget).cast::(); let mut i = 0; // FIXME: this conversion is very hacky let v = br"\??\"; @@ -1367,7 +1368,7 @@ fn symlink_junction_inner(original: &Path, junction: &Path) -> io::Result<()> { cvt(c::DeviceIoControl( h as *mut _, c::FSCTL_SET_REPARSE_POINT, - data.as_ptr() as *mut _, + data_ptr.cast(), (*db).ReparseDataLength + 8, ptr::null_mut(), 0, From 5c3490c90186a1b4d1ac66b57ddede55410409e1 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Tue, 30 Aug 2022 01:15:59 -0700 Subject: [PATCH 3/4] Replace `AlignedAs` with a more specific `Align8` type --- library/std/src/sys/windows/fs.rs | 27 +++++++++++++------------ library/std/src/sys/windows/mod.rs | 27 ++++++------------------- library/std/src/sys/windows/os/tests.rs | 18 ----------------- 3 files changed, 20 insertions(+), 52 deletions(-) diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs index 1545ba0d023fa..c702b8e744c89 100644 --- 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, AlignedAs}; +use crate::sys::{c, cvt, Align8}; use crate::sys_common::{AsInner, FromInner, IntoInner}; use crate::thread; @@ -47,9 +47,6 @@ pub struct ReadDir { first: Option, } -type AlignedReparseBuf = - AlignedAs; - struct FindNextFileHandle(c::HANDLE); unsafe impl Send for FindNextFileHandle {} @@ -329,7 +326,7 @@ 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 = AlignedReparseBuf::new([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]); + let mut b = Align8([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]); if let Ok((_, buf)) = self.reparse_point(&mut b) { reparse_tag = (*buf).ReparseTag; } @@ -392,7 +389,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 = AlignedReparseBuf::new([0; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]); + let mut b = Align8([0; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]); if let Ok((_, buf)) = self.reparse_point(&mut b) { attr.reparse_tag = (*buf).ReparseTag; } @@ -466,28 +463,32 @@ impl File { // avoid narrowing provenance to the actual `REPARSE_DATA_BUFFER`. fn reparse_point( &self, - space: &mut AlignedReparseBuf, + space: &mut Align8<[u8]>, ) -> io::Result<(c::DWORD, *const c::REPARSE_DATA_BUFFER)> { unsafe { let mut bytes = 0; cvt({ + // Grab this in advance to avoid it invalidating the pointer + // we get from `space.0.as_mut_ptr()`. + let len = space.0.len(); c::DeviceIoControl( self.handle.as_raw_handle(), c::FSCTL_GET_REPARSE_POINT, ptr::null_mut(), 0, - space.value.as_mut_ptr() as *mut _, - space.value.len() as c::DWORD, + space.0.as_mut_ptr().cast(), + len as c::DWORD, &mut bytes, ptr::null_mut(), ) })?; - Ok((bytes, space.value.as_ptr().cast::())) + const _: () = assert!(core::mem::align_of::() <= 8); + Ok((bytes, space.0.as_ptr().cast::())) } } fn readlink(&self) -> io::Result { - let mut space = AlignedReparseBuf::new([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]); + let mut space = Align8([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 { @@ -1345,8 +1346,8 @@ fn symlink_junction_inner(original: &Path, junction: &Path) -> io::Result<()> { let h = f.as_inner().as_raw_handle(); unsafe { - let mut data = AlignedReparseBuf::new([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]); - let data_ptr = data.value.as_mut_ptr(); + let mut data = Align8([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]); + let data_ptr = data.0.as_mut_ptr(); let db = data_ptr.cast::(); let buf = ptr::addr_of_mut!((*db).ReparseTarget).cast::(); let mut i = 0; diff --git a/library/std/src/sys/windows/mod.rs b/library/std/src/sys/windows/mod.rs index fbbc5cb79fb8b..340cae4066bf4 100644 --- a/library/std/src/sys/windows/mod.rs +++ b/library/std/src/sys/windows/mod.rs @@ -330,25 +330,10 @@ pub fn abort_internal() -> ! { crate::intrinsics::abort(); } -/// Used for some win32 buffers which are stack allocated, for example: -/// `AlignedAs` -#[repr(C)] +/// Align the inner value to 8 bytes. +/// +/// This is enough for almost all of the buffers we're likely to work with in +/// the Windows APIs we use. +#[repr(C, align(8))] #[derive(Copy, Clone)] -pub struct AlignedAs { - /// Use `[Aligner; 0]` as a sort of `PhantomAlignNextField`. 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 AlignedAs { - // 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 } - } -} +pub(crate) struct Align8(pub T); diff --git a/library/std/src/sys/windows/os/tests.rs b/library/std/src/sys/windows/os/tests.rs index 532be0cf083ba..458d6e11c2098 100644 --- a/library/std/src/sys/windows/os/tests.rs +++ b/library/std/src/sys/windows/os/tests.rs @@ -11,21 +11,3 @@ 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; - assert!(size_of::() >= c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE); - assert_eq!(align_of::(), align_of::()); - let a = AlignedReparseBuf::new([0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]); - // Quick and dirty offsetof check. - assert_eq!(addr_of!(a).cast::(), addr_of!(a.value).cast::()); - // Smoke check that it's actually aligned. - assert!(addr_of!(a.value).is_aligned_to(align_of::())); -} From c41f21b3e4e992d5c5c11dae880dc81192a12653 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Tue, 30 Aug 2022 01:45:30 -0700 Subject: [PATCH 4/4] Fix UB in Windows `DirBuffIter` (provenance and alignment) --- library/std/src/sys/windows/fs.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs index c702b8e744c89..0aa7c50ded1c7 100644 --- a/library/std/src/sys/windows/fs.rs +++ b/library/std/src/sys/windows/fs.rs @@ -658,18 +658,18 @@ impl File { /// A buffer for holding directory entries. struct DirBuff { - buffer: Vec, + buffer: Box>, } impl DirBuff { + const BUFFER_SIZE: usize = 1024; fn new() -> Self { - const BUFFER_SIZE: usize = 1024; - Self { buffer: vec![0_u8; BUFFER_SIZE] } + Self { buffer: Box::new(Align8([0u8; Self::BUFFER_SIZE])) } } fn capacity(&self) -> usize { - self.buffer.len() + self.buffer.0.len() } fn as_mut_ptr(&mut self) -> *mut u8 { - self.buffer.as_mut_ptr().cast() + self.buffer.0.as_mut_ptr().cast() } /// Returns a `DirBuffIter`. fn iter(&self) -> DirBuffIter<'_> { @@ -678,7 +678,7 @@ impl DirBuff { } impl AsRef<[u8]> for DirBuff { fn as_ref(&self) -> &[u8] { - &self.buffer + &self.buffer.0 } } @@ -706,9 +706,12 @@ impl<'a> Iterator for DirBuffIter<'a> { // used to get the file name slice. let (name, is_directory, next_entry) = unsafe { let info = buffer.as_ptr().cast::(); + // Guaranteed to be aligned in documentation for + // https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_id_both_dir_info + assert!(info.is_aligned()); let next_entry = (*info).NextEntryOffset as usize; let name = crate::slice::from_raw_parts( - (*info).FileName.as_ptr().cast::(), + ptr::addr_of!((*info).FileName).cast::(), (*info).FileNameLength as usize / size_of::(), ); let is_directory = ((*info).FileAttributes & c::FILE_ATTRIBUTE_DIRECTORY) != 0;