Skip to content

Commit

Permalink
Ensure Ptr/PtrMut/OwningPtr are aligned when casting in debug builds (b…
Browse files Browse the repository at this point in the history
…evyengine#7117)

# Objective
Improve safety testing when using `bevy_ptr` types. This is a follow-up to bevyengine#7113.

## Solution
Add a debug-only assertion that pointers are aligned when casting to a concrete type. This should very quickly catch any unsoundness from unaligned pointers, even without miri. However, this can have a large negative perf impact on debug builds.

---

## Changelog
Added: `Ptr::deref` will now panic in debug builds if the pointer is not aligned.
Added: `PtrMut::deref_mut` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::read` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::drop_as` will now panic in debug builds if the pointer is not aligned.
  • Loading branch information
james7132 authored and alradish committed Jan 22, 2023
1 parent 2ebbbb0 commit 3d30dc8
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 8 deletions.
7 changes: 4 additions & 3 deletions crates/bevy_ecs/src/storage/blob_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,19 @@ impl BlobVec {
drop: Option<unsafe fn(OwningPtr<'_>)>,
capacity: usize,
) -> BlobVec {
let align = NonZeroUsize::new(item_layout.align()).expect("alignment must be > 0");
let data = bevy_ptr::dangling_with_align(align);
if item_layout.size() == 0 {
let align = NonZeroUsize::new(item_layout.align()).expect("alignment must be > 0");
BlobVec {
data: bevy_ptr::dangling_with_align(align),
data,
capacity: usize::MAX,
len: 0,
item_layout,
drop,
}
} else {
let mut blob_vec = BlobVec {
data: NonNull::dangling(),
data,
capacity: 0,
len: 0,
item_layout,
Expand Down
49 changes: 44 additions & 5 deletions crates/bevy_ptr/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ impl<'a, A: IsAligned> Ptr<'a, A> {
/// for the pointee type `T`.
#[inline]
pub unsafe fn deref<T>(self) -> &'a T {
&*self.as_ptr().cast()
&*self.as_ptr().cast::<T>().debug_ensure_aligned()
}

/// Gets the underlying pointer, erasing the associated lifetime.
Expand Down Expand Up @@ -218,7 +218,7 @@ impl<'a, A: IsAligned> PtrMut<'a, A> {
/// for the pointee type `T`.
#[inline]
pub unsafe fn deref_mut<T>(self) -> &'a mut T {
&mut *self.as_ptr().cast()
&mut *self.as_ptr().cast::<T>().debug_ensure_aligned()
}

/// Gets the underlying pointer, erasing the associated lifetime.
Expand Down Expand Up @@ -287,7 +287,7 @@ impl<'a, A: IsAligned> OwningPtr<'a, A> {
/// for the pointee type `T`.
#[inline]
pub unsafe fn read<T>(self) -> T {
self.as_ptr().cast::<T>().read()
self.as_ptr().cast::<T>().debug_ensure_aligned().read()
}

/// Consumes the [`OwningPtr`] to drop the underlying data of type `T`.
Expand All @@ -298,7 +298,10 @@ impl<'a, A: IsAligned> OwningPtr<'a, A> {
/// for the pointee type `T`.
#[inline]
pub unsafe fn drop_as<T>(self) {
self.as_ptr().cast::<T>().drop_in_place();
self.as_ptr()
.cast::<T>()
.debug_ensure_aligned()
.drop_in_place();
}

/// Gets the underlying pointer, erasing the associated lifetime.
Expand Down Expand Up @@ -364,9 +367,10 @@ impl<'a, T> Copy for ThinSlicePtr<'a, T> {}
impl<'a, T> From<&'a [T]> for ThinSlicePtr<'a, T> {
#[inline]
fn from(slice: &'a [T]) -> Self {
let ptr = slice.as_ptr() as *mut T;
Self {
// SAFETY: a reference can never be null
ptr: unsafe { NonNull::new_unchecked(slice.as_ptr() as *mut T) },
ptr: unsafe { NonNull::new_unchecked(ptr.debug_ensure_aligned()) },
#[cfg(debug_assertions)]
len: slice.len(),
_marker: PhantomData,
Expand All @@ -377,6 +381,7 @@ impl<'a, T> From<&'a [T]> for ThinSlicePtr<'a, T> {
/// Creates a dangling pointer with specified alignment.
/// See [`NonNull::dangling`].
pub fn dangling_with_align(align: NonZeroUsize) -> NonNull<u8> {
debug_assert!(align.is_power_of_two(), "Alignment must be power of two.");
// SAFETY: The pointer will not be null, since it was created
// from the address of a `NonZeroUsize`.
unsafe { NonNull::new_unchecked(align.get() as *mut u8) }
Expand Down Expand Up @@ -429,3 +434,37 @@ impl<'a, T> UnsafeCellDeref<'a, T> for &'a UnsafeCell<T> {
self.get().read()
}
}

trait DebugEnsureAligned {
fn debug_ensure_aligned(self) -> Self;
}

// Disable this for miri runs as it already checks if pointer to reference
// casts are properly aligned.
#[cfg(all(debug_assertions, not(miri)))]
impl<T: Sized> DebugEnsureAligned for *mut T {
#[track_caller]
fn debug_ensure_aligned(self) -> Self {
let align = core::mem::align_of::<T>();
// Implemenation shamelessly borrowed from the currently unstable
// ptr.is_aligned_to.
//
// Replace once https://github.com/rust-lang/rust/issues/96284 is stable.
assert!(
self as usize & (align - 1) == 0,
"pointer is not aligned. Address {:p} does not have alignemnt {} for type {}",
self,
align,
core::any::type_name::<T>(),
);
self
}
}

#[cfg(any(not(debug_assertions), miri))]
impl<T: Sized> DebugEnsureAligned for *mut T {
#[inline(always)]
fn debug_ensure_aligned(self) -> Self {
self
}
}

0 comments on commit 3d30dc8

Please sign in to comment.