From 3fc35b5b935e390c61ea2bbf744838b2632b2df1 Mon Sep 17 00:00:00 2001 From: BlackHoleFox Date: Thu, 25 Aug 2022 15:43:59 -0500 Subject: Use getentropy when possible on all Apple platforms --- library/std/src/sys/unix/rand.rs | 94 ++++++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 38 deletions(-) (limited to 'library/std/src') diff --git a/library/std/src/sys/unix/rand.rs b/library/std/src/sys/unix/rand.rs index a6fe07873d7..40885417308 100644 --- a/library/std/src/sys/unix/rand.rs +++ b/library/std/src/sys/unix/rand.rs @@ -137,11 +137,9 @@ mod imp { } } -#[cfg(target_os = "macos")] +#[cfg(any(target_os = "macos", target_os = "ios", target_os = "watchos"))] mod imp { - use crate::fs::File; - use crate::io::Read; - use crate::sys::os::errno; + use crate::io; use crate::sys::weak::weak; use libc::{c_int, c_void, size_t}; @@ -155,7 +153,7 @@ mod imp { for s in v.chunks_mut(256) { let ret = unsafe { f(s.as_mut_ptr() as *mut c_void, s.len()) }; if ret == -1 { - panic!("unexpected getentropy error: {}", errno()); + panic!("unexpected getentropy error: {}", io::Error::last_os_error()); } } true @@ -163,14 +161,64 @@ mod imp { .unwrap_or(false) } + #[cfg(target_os = "macos")] + fn fallback_fill_bytes(v: &mut [u8]) { + use crate::fs::File; + use crate::io::Read; + + let mut file = File::open("/dev/urandom").expect("failed to open /dev/urandom"); + file.read_exact(v).expect("failed to read /dev/urandom") + } + + // On iOS and MacOS `SecRandomCopyBytes` calls `CCRandomCopyBytes` with + // `kCCRandomDefault`. `CCRandomCopyBytes` manages a CSPRNG which is seeded + // from `/dev/random` and which runs on its own thread accessed via GCD. + // + // This is very heavyweight compared to the alternatives, but they may not be usable: + // - `getentropy` was added in iOS 10, but we support a minimum of iOS 7 + // - `/dev/urandom` is not accessible inside the iOS app sandbox. + // + // Therefore `SecRandomCopyBytes` is only used on older iOS versions where no + // better options are present. + #[cfg(target_os = "ios")] + fn fallback_fill_bytes(v: &mut [u8]) { + use crate::ptr; + + enum SecRandom {} + + #[allow(non_upper_case_globals)] + const kSecRandomDefault: *const SecRandom = ptr::null(); + + extern "C" { + fn SecRandomCopyBytes(rnd: *const SecRandom, count: size_t, bytes: *mut u8) -> c_int; + } + + let ret = unsafe { SecRandomCopyBytes(kSecRandomDefault, v.len(), v.as_mut_ptr()) }; + if ret == -1 { + panic!("couldn't generate random bytes: {}", io::Error::last_os_error()); + } + } + + // All supported versions of watchOS (>= 5) have support for `getentropy`. + #[cfg(target_os = "watchos")] + #[cold] + fn fallback_fill_bytes(_: &mut [u8]) { + unreachable!() + } + pub fn fill_bytes(v: &mut [u8]) { if getentropy_fill_bytes(v) { return; } - // for older macos which doesn't support getentropy - let mut file = File::open("/dev/urandom").expect("failed to open /dev/urandom"); - file.read_exact(v).expect("failed to read /dev/urandom") + // Older macOS versions (< 10.12) don't support `getentropy`. Fallback to + // reading from `/dev/urandom` on these systems. + // + // Older iOS versions (< 10) don't support it either. Fallback to + // `SecRandomCopyBytes` on these systems. On watchOS, this is unreachable + // because the minimum supported version is 5 while `getentropy` became accessible + // in 3. + fallback_fill_bytes(v) } } @@ -189,36 +237,6 @@ mod imp { } } -// On iOS and MacOS `SecRandomCopyBytes` calls `CCRandomCopyBytes` with -// `kCCRandomDefault`. `CCRandomCopyBytes` manages a CSPRNG which is seeded -// from `/dev/random` and which runs on its own thread accessed via GCD. -// This seems needlessly heavyweight for the purposes of generating two u64s -// once per thread in `hashmap_random_keys`. Therefore `SecRandomCopyBytes` is -// only used on iOS where direct access to `/dev/urandom` is blocked by the -// sandbox. -#[cfg(any(target_os = "ios", target_os = "watchos"))] -mod imp { - use crate::io; - use crate::ptr; - use libc::{c_int, size_t}; - - enum SecRandom {} - - #[allow(non_upper_case_globals)] - const kSecRandomDefault: *const SecRandom = ptr::null(); - - extern "C" { - fn SecRandomCopyBytes(rnd: *const SecRandom, count: size_t, bytes: *mut u8) -> c_int; - } - - pub fn fill_bytes(v: &mut [u8]) { - let ret = unsafe { SecRandomCopyBytes(kSecRandomDefault, v.len(), v.as_mut_ptr()) }; - if ret == -1 { - panic!("couldn't generate random bytes: {}", io::Error::last_os_error()); - } - } -} - #[cfg(any(target_os = "freebsd", target_os = "netbsd"))] mod imp { use crate::ptr; -- cgit 1.4.1-3-g733a5 From 7bb47a6f38d2c4673663fa1370817c7fb5e682c1 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Sat, 20 Aug 2022 08:03:33 +0100 Subject: Reinstate preloading of some dll imports --- library/std/src/sys/windows/c.rs | 3 -- library/std/src/sys/windows/compat.rs | 82 ++++++++++++++++++++++------------- 2 files changed, 52 insertions(+), 33 deletions(-) (limited to 'library/std/src') diff --git a/library/std/src/sys/windows/c.rs b/library/std/src/sys/windows/c.rs index ef3f6a9ba17..891d7e855f0 100644 --- a/library/std/src/sys/windows/c.rs +++ b/library/std/src/sys/windows/c.rs @@ -228,8 +228,6 @@ pub const IPV6_ADD_MEMBERSHIP: c_int = 12; pub const IPV6_DROP_MEMBERSHIP: c_int = 13; pub const MSG_PEEK: c_int = 0x2; -pub const LOAD_LIBRARY_SEARCH_SYSTEM32: u32 = 0x800; - #[repr(C)] #[derive(Copy, Clone)] pub struct linger { @@ -1032,7 +1030,6 @@ extern "system" { pub fn GetProcAddress(handle: HMODULE, name: LPCSTR) -> *mut c_void; pub fn GetModuleHandleA(lpModuleName: LPCSTR) -> HMODULE; pub fn GetModuleHandleW(lpModuleName: LPCWSTR) -> HMODULE; - pub fn LoadLibraryExA(lplibfilename: *const i8, hfile: HANDLE, dwflags: u32) -> HINSTANCE; pub fn GetSystemTimeAsFileTime(lpSystemTimeAsFileTime: LPFILETIME); pub fn GetSystemInfo(lpSystemInfo: LPSYSTEM_INFO); diff --git a/library/std/src/sys/windows/compat.rs b/library/std/src/sys/windows/compat.rs index 9c8ddc3aa1d..7dff81ecb8d 100644 --- a/library/std/src/sys/windows/compat.rs +++ b/library/std/src/sys/windows/compat.rs @@ -21,9 +21,52 @@ use crate::ffi::{c_void, CStr}; use crate::ptr::NonNull; -use crate::sync::atomic::{AtomicBool, Ordering}; +use crate::sync::atomic::Ordering; use crate::sys::c; +// This uses a static initializer to preload some imported functions. +// The CRT (C runtime) executes static initializers before `main` +// is called (for binaries) and before `DllMain` is called (for DLLs). +// +// It works by contributing a global symbol to the `.CRT$XCT` section. +// The linker builds a table of all static initializer functions. +// The CRT startup code then iterates that table, calling each +// initializer function. +// +// NOTE: User code should instead use .CRT$XCU to reliably run after std's initializer. +// If you're reading this and would like a guarantee here, please +// file an issue for discussion; currently we don't guarantee any functionality +// before main. +// See https://docs.microsoft.com/en-us/cpp/c-runtime-library/crt-initialization?view=msvc-170 +#[used] +#[link_section = ".CRT$XCT"] +static INIT_TABLE_ENTRY: unsafe extern "C" fn() = init; + +/// Preload some imported functions. +/// +/// Note that any functions included here will be unconditionally loaded in +/// the final binary, regardless of whether or not they're actually used. +/// +/// Therefore, this should be limited to `compat_fn_optional` functions which +/// must be preloaded or any functions where lazier loading demonstrates a +/// negative performance impact in practical situations. +/// +/// Currently we only preload `WaitOnAddress` and `WakeByAddressSingle`. +unsafe extern "C" fn init() { + // In an exe this code is executed before main() so is single threaded. + // In a DLL the system's loader lock will be held thereby synchronizing + // access. So the same best practices apply here as they do to running in DllMain: + // https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-best-practices + // + // DO NOT do anything interesting or complicated in this function! DO NOT call + // any Rust functions or CRT functions if those functions touch any global state, + // because this function runs during global initialization. For example, DO NOT + // do any dynamic allocation, don't call LoadLibrary, etc. + + // Attempt to preload the synch functions. + load_synch_functions(); +} + /// Helper macro for creating CStrs from literals and symbol names. macro_rules! ansi_str { (sym $ident:ident) => {{ @@ -75,20 +118,6 @@ impl Module { NonNull::new(module).map(Self) } - /// Load the library (if not already loaded) - /// - /// # Safety - /// - /// The module must not be unloaded. - pub unsafe fn load_system_library(name: &CStr) -> Option { - let module = c::LoadLibraryExA( - name.as_ptr(), - crate::ptr::null_mut(), - c::LOAD_LIBRARY_SEARCH_SYSTEM32, - ); - NonNull::new(module).map(Self) - } - // Try to get the address of a function. pub fn proc_address(self, name: &CStr) -> Option> { // SAFETY: @@ -182,14 +211,10 @@ macro_rules! compat_fn_optional { #[inline(always)] pub fn option() -> Option { - let f = PTR.load(Ordering::Acquire); - if !f.is_null() { Some(unsafe { mem::transmute(f) }) } else { try_load() } - } - - #[cold] - fn try_load() -> Option { - $load_functions; - NonNull::new(PTR.load(Ordering::Acquire)).map(|f| unsafe { mem::transmute(f) }) + // Miri does not understand the way we do preloading + // therefore load the function here instead. + #[cfg(miri)] $load_functions; + NonNull::new(PTR.load(Ordering::Relaxed)).map(|f| unsafe { mem::transmute(f) }) } } )+ @@ -205,17 +230,14 @@ pub(super) fn load_synch_functions() { // Try loading the library and all the required functions. // If any step fails, then they all fail. - let library = unsafe { Module::load_system_library(MODULE_NAME) }?; + let library = unsafe { Module::new(MODULE_NAME) }?; let wait_on_address = library.proc_address(WAIT_ON_ADDRESS)?; let wake_by_address_single = library.proc_address(WAKE_BY_ADDRESS_SINGLE)?; - c::WaitOnAddress::PTR.store(wait_on_address.as_ptr(), Ordering::Release); - c::WakeByAddressSingle::PTR.store(wake_by_address_single.as_ptr(), Ordering::Release); + c::WaitOnAddress::PTR.store(wait_on_address.as_ptr(), Ordering::Relaxed); + c::WakeByAddressSingle::PTR.store(wake_by_address_single.as_ptr(), Ordering::Relaxed); Some(()) } - // Try to load the module but skip loading if a previous attempt failed. - static LOAD_MODULE: AtomicBool = AtomicBool::new(true); - let module_loaded = LOAD_MODULE.load(Ordering::Acquire) && try_load().is_some(); - LOAD_MODULE.store(module_loaded, Ordering::Release) + try_load(); } -- cgit 1.4.1-3-g733a5 From 1b8025a24c4b063d2566671f0664e5dfc263c2a4 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Mon, 29 Aug 2022 15:36:01 -0700 Subject: 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(-) (limited to 'library/std/src') diff --git a/library/std/src/sys/windows/c.rs b/library/std/src/sys/windows/c.rs index ef3f6a9ba17..865e2ad4b43 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 9653b1abfc3..01f914617a0 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 a9846a48488..fbbc5cb79fb 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 458d6e11c20..532be0cf083 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::())); +} -- cgit 1.4.1-3-g733a5 From d9c760db43d8ab701a71f633d820efc72d8cedea Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Tue, 30 Aug 2022 00:16:53 -0700 Subject: Fix UWP and use `AlignedReparseBuf` in `symlink_junction_inner` --- library/std/src/sys/windows/fs.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) (limited to 'library/std/src') diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs index 01f914617a0..1545ba0d023 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, -- cgit 1.4.1-3-g733a5 From 5c3490c90186a1b4d1ac66b57ddede55410409e1 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Tue, 30 Aug 2022 01:15:59 -0700 Subject: 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(-) (limited to 'library/std/src') diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs index 1545ba0d023..c702b8e744c 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 fbbc5cb79fb..340cae4066b 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 532be0cf083..458d6e11c20 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::())); -} -- cgit 1.4.1-3-g733a5 From c41f21b3e4e992d5c5c11dae880dc81192a12653 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Tue, 30 Aug 2022 01:45:30 -0700 Subject: Fix UB in Windows `DirBuffIter` (provenance and alignment) --- library/std/src/sys/windows/fs.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) (limited to 'library/std/src') diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs index c702b8e744c..0aa7c50ded1 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; -- cgit 1.4.1-3-g733a5