From 56c363b43e04803e891f242e78efc0c53230314f Mon Sep 17 00:00:00 2001 From: Strophox Date: Thu, 23 May 2024 14:44:41 +0200 Subject: [PATCH 1/3] fix alloc_bytes (always allocate at least 1B) --- src/tools/miri/src/alloc_bytes.rs | 37 +++++++++++++------------------ src/tools/miri/src/lib.rs | 1 - 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/src/tools/miri/src/alloc_bytes.rs b/src/tools/miri/src/alloc_bytes.rs index 7952abdf9f400..6d35b7997e58c 100644 --- a/src/tools/miri/src/alloc_bytes.rs +++ b/src/tools/miri/src/alloc_bytes.rs @@ -13,10 +13,7 @@ pub struct MiriAllocBytes { /// Stored layout information about the allocation. layout: alloc::Layout, /// Pointer to the allocation contents. - /// Invariant: - /// * If `self.layout.size() == 0`, then `self.ptr` is some suitably aligned pointer - /// without provenance (and no actual memory was allocated). - /// * Otherwise, `self.ptr` points to memory allocated with `self.layout`. + /// Invariant: `self.ptr` points to memory allocated with `self.layout`. ptr: *mut u8, } @@ -30,10 +27,8 @@ impl Clone for MiriAllocBytes { impl Drop for MiriAllocBytes { fn drop(&mut self) { - if self.layout.size() != 0 { - // SAFETY: Invariant, `self.ptr` points to memory allocated with `self.layout`. - unsafe { alloc::dealloc(self.ptr, self.layout) } - } + // SAFETY: Invariant, `self.ptr` points to memory allocated with `self.layout`. + unsafe { alloc::dealloc(self.ptr, self.layout) } } } @@ -58,25 +53,23 @@ impl std::ops::DerefMut for MiriAllocBytes { impl MiriAllocBytes { /// This method factors out how a `MiriAllocBytes` object is allocated, /// specifically given an allocation function `alloc_fn`. - /// `alloc_fn` is only used if `size != 0`. - /// Returns `Err(layout)` if the allocation function returns a `ptr` that is `ptr.is_null()`. + /// `alloc_fn` is only used with `size != 0`. + /// Returns `Err(layout)` if the allocation function returns a `ptr` where `ptr.is_null()`. fn alloc_with( size: usize, align: usize, alloc_fn: impl FnOnce(Layout) -> *mut u8, ) -> Result { + // When size is 0 we allocate 1 byte anyway, so addresses don't possibly overlap. + let size = if size == 0 { 1 } else { size }; let layout = Layout::from_size_align(size, align).unwrap(); - let ptr = if size == 0 { - std::ptr::without_provenance_mut(align) + let ptr = alloc_fn(layout); + if ptr.is_null() { + Err(layout) } else { - let ptr = alloc_fn(layout); - if ptr.is_null() { - return Err(layout); - } - ptr - }; - // SAFETY: All `MiriAllocBytes` invariants are fulfilled. - Ok(Self { ptr, layout }) + // SAFETY: All `MiriAllocBytes` invariants are fulfilled. + Ok(Self { ptr, layout }) + } } } @@ -85,7 +78,7 @@ impl AllocBytes for MiriAllocBytes { let slice = slice.into(); let size = slice.len(); let align = align.bytes_usize(); - // SAFETY: `alloc_fn` will only be used if `size != 0`. + // SAFETY: `alloc_fn` will only be used with `size != 0`. let alloc_fn = |layout| unsafe { alloc::alloc(layout) }; let alloc_bytes = MiriAllocBytes::alloc_with(size, align, alloc_fn) .unwrap_or_else(|layout| alloc::handle_alloc_error(layout)); @@ -98,7 +91,7 @@ impl AllocBytes for MiriAllocBytes { fn zeroed(size: Size, align: Align) -> Option { let size = size.bytes_usize(); let align = align.bytes_usize(); - // SAFETY: `alloc_fn` will only be used if `size != 0`. + // SAFETY: `alloc_fn` will only be used with `size != 0`. let alloc_fn = |layout| unsafe { alloc::alloc_zeroed(layout) }; MiriAllocBytes::alloc_with(size, align, alloc_fn).ok() } diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index e4879f2f53155..d23a44c376bc4 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -13,7 +13,6 @@ #![feature(lint_reasons)] #![feature(trait_upcasting)] #![feature(strict_overflow_ops)] -#![feature(strict_provenance)] // Configure clippy and other lints #![allow( clippy::collapsible_else_if, From 1b374dfd9bcb234f44270ebb8f3b957d2e8a7820 Mon Sep 17 00:00:00 2001 From: Strophox Date: Thu, 23 May 2024 16:00:23 +0200 Subject: [PATCH 2/3] differentiate between layout and alloc_layout --- src/tools/miri/src/alloc_bytes.rs | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/tools/miri/src/alloc_bytes.rs b/src/tools/miri/src/alloc_bytes.rs index 6d35b7997e58c..0ae581dacdcef 100644 --- a/src/tools/miri/src/alloc_bytes.rs +++ b/src/tools/miri/src/alloc_bytes.rs @@ -13,7 +13,10 @@ pub struct MiriAllocBytes { /// Stored layout information about the allocation. layout: alloc::Layout, /// Pointer to the allocation contents. - /// Invariant: `self.ptr` points to memory allocated with `self.layout`. + /// Invariant: + /// * If `self.layout.size() == 0`, then `self.ptr` is some suitably aligned pointer + /// that was allocated with the same layout but `size == 1`. + /// * Otherwise, `self.ptr` points to memory allocated with `self.layout`. ptr: *mut u8, } @@ -27,8 +30,13 @@ impl Clone for MiriAllocBytes { impl Drop for MiriAllocBytes { fn drop(&mut self) { + let alloc_layout = if self.layout.size() == 0 { + Layout::from_size_align(1, self.layout.align()).unwrap() + } else { + self.layout + }; // SAFETY: Invariant, `self.ptr` points to memory allocated with `self.layout`. - unsafe { alloc::dealloc(self.ptr, self.layout) } + unsafe { alloc::dealloc(self.ptr, alloc_layout) } } } @@ -51,21 +59,21 @@ impl std::ops::DerefMut for MiriAllocBytes { } impl MiriAllocBytes { - /// This method factors out how a `MiriAllocBytes` object is allocated, - /// specifically given an allocation function `alloc_fn`. - /// `alloc_fn` is only used with `size != 0`. - /// Returns `Err(layout)` if the allocation function returns a `ptr` where `ptr.is_null()`. + /// This method factors out how a `MiriAllocBytes` object is allocated, given a specific allocation function. + /// If `size == 0` we allocate using a different `alloc_layout` with `size = 1`, to ensure each allocation has a unique address. + /// Returns `Err(alloc_layout)` if the allocation function returns a `ptr` where `ptr.is_null()`. fn alloc_with( size: usize, align: usize, alloc_fn: impl FnOnce(Layout) -> *mut u8, ) -> Result { - // When size is 0 we allocate 1 byte anyway, so addresses don't possibly overlap. - let size = if size == 0 { 1 } else { size }; let layout = Layout::from_size_align(size, align).unwrap(); - let ptr = alloc_fn(layout); + // When size is 0 we allocate 1 byte anyway, to ensure each allocation has a unique address. + let alloc_layout = + if size == 0 { Layout::from_size_align(1, align).unwrap() } else { layout }; + let ptr = alloc_fn(alloc_layout); if ptr.is_null() { - Err(layout) + Err(alloc_layout) } else { // SAFETY: All `MiriAllocBytes` invariants are fulfilled. Ok(Self { ptr, layout }) From b84620ff17b386969052d26b2868aa7e2557be79 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 24 May 2024 10:10:07 +0200 Subject: [PATCH 3/3] extend comments --- src/tools/miri/src/alloc_bytes.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/src/alloc_bytes.rs b/src/tools/miri/src/alloc_bytes.rs index 0ae581dacdcef..97841a05cdebf 100644 --- a/src/tools/miri/src/alloc_bytes.rs +++ b/src/tools/miri/src/alloc_bytes.rs @@ -14,8 +14,7 @@ pub struct MiriAllocBytes { layout: alloc::Layout, /// Pointer to the allocation contents. /// Invariant: - /// * If `self.layout.size() == 0`, then `self.ptr` is some suitably aligned pointer - /// that was allocated with the same layout but `size == 1`. + /// * If `self.layout.size() == 0`, then `self.ptr` was allocated with the equivalent layout with size 1. /// * Otherwise, `self.ptr` points to memory allocated with `self.layout`. ptr: *mut u8, } @@ -30,6 +29,8 @@ impl Clone for MiriAllocBytes { impl Drop for MiriAllocBytes { fn drop(&mut self) { + // We have to reconstruct the actual layout used for allocation. + // (`Deref` relies on `size` so we can't just always set it to at least 1.) let alloc_layout = if self.layout.size() == 0 { Layout::from_size_align(1, self.layout.align()).unwrap() } else {