Skip to content

Commit

Permalink
Auto merge of #3625 - Strophox:miri-allocation-fix, r=RalfJung
Browse files Browse the repository at this point in the history
Bugfix `MiriAllocBytes` to guarantee different addresses

Fix in `alloc_bytes.rs` following #3526

Currently when an allocation of `size == 0` is requested we return a `std::ptr::without_provenance_mut(align)`, but this means returned `ptr`s may overlap, which breaks things.
  • Loading branch information
bors committed May 24, 2024
2 parents 74aa018 + 120efdc commit d7ddcaf
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 23 deletions.
46 changes: 24 additions & 22 deletions src/alloc_bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// without provenance (and no actual memory was allocated).
/// * 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,
}
Expand All @@ -30,10 +29,15 @@ 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) }
}
// 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 {
self.layout
};
// SAFETY: Invariant, `self.ptr` points to memory allocated with `self.layout`.
unsafe { alloc::dealloc(self.ptr, alloc_layout) }
}
}

Expand All @@ -56,27 +60,25 @@ 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()`.
/// 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<MiriAllocBytes, Layout> {
let layout = Layout::from_size_align(size, align).unwrap();
let ptr = if size == 0 {
std::ptr::without_provenance_mut(align)
// 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(alloc_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 })
}
}
}

Expand All @@ -85,7 +87,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));
Expand All @@ -98,7 +100,7 @@ impl AllocBytes for MiriAllocBytes {
fn zeroed(size: Size, align: Align) -> Option<Self> {
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()
}
Expand Down
1 change: 0 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit d7ddcaf

Please sign in to comment.