From 513cf03aa3424a3a6cdb701ded919e69cf4380ba Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 6 Jan 2023 20:04:04 -0800 Subject: [PATCH 01/13] Ensure Ptr/PtrMut/OwningPtr are aligned when casting in debug modes --- crates/bevy_ptr/src/lib.rs | 57 ++++++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index bf38fd5ff1d9f..d2a858482381e 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -4,7 +4,8 @@ use core::fmt::{self, Formatter, Pointer}; use core::{ - cell::UnsafeCell, marker::PhantomData, mem::ManuallyDrop, num::NonZeroUsize, ptr::NonNull, + alloc::Layout, cell::UnsafeCell, marker::PhantomData, mem::ManuallyDrop, num::NonZeroUsize, + ptr::NonNull, }; /// Type-erased borrow of some unknown type chosen when constructing this type. @@ -121,11 +122,15 @@ impl<'a> Ptr<'a> { /// Transforms this [`Ptr`] into a `&T` with the same lifetime /// + /// # Panics + /// In debug builds, this function will panic if the pointer is not properly + /// aligned for `T`. + /// /// # Safety /// Must point to a valid `T` #[inline] pub unsafe fn deref(self) -> &'a T { - &*self.as_ptr().cast() + &*self.as_ptr().cast::().ensure_aligned() } /// Gets the underlying pointer, erasing the associated lifetime. @@ -160,11 +165,15 @@ impl<'a> PtrMut<'a> { /// Transforms this [`PtrMut`] into a `&mut T` with the same lifetime /// + /// # Panics + /// In debug builds, this function will panic if the pointer is not properly + /// aligned for `T`. + /// /// # Safety /// Must point to a valid `T` #[inline] pub unsafe fn deref_mut(self) -> &'a mut T { - &mut *self.as_ptr().cast() + &mut *self.as_ptr().cast::().ensure_aligned() } /// Gets the underlying pointer, erasing the associated lifetime. @@ -213,20 +222,28 @@ impl<'a> OwningPtr<'a> { /// Consumes the [`OwningPtr`] to obtain ownership of the underlying data of type `T`. /// + /// # Panics + /// In debug builds, this function will panic if the pointer is not properly + /// aligned for `T`. + /// /// # Safety /// Must point to a valid `T`. #[inline] pub unsafe fn read(self) -> T { - self.as_ptr().cast::().read() + self.as_ptr().cast::().ensure_aligned().read() } /// Consumes the [`OwningPtr`] to drop the underlying data of type `T`. /// + /// # Panics + /// In debug builds, this function will panic if the pointer is not properly + /// aligned for `T`. + /// /// # Safety /// Must point to a valid `T`. #[inline] pub unsafe fn drop_as(self) { - self.as_ptr().cast::().drop_in_place(); + self.as_ptr().cast::().ensure_aligned().drop_in_place(); } /// Gets the underlying pointer, erasing the associated lifetime. @@ -292,9 +309,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.ensure_aligned()) }, #[cfg(debug_assertions)] len: slice.len(), _marker: PhantomData, @@ -357,3 +375,30 @@ impl<'a, T> UnsafeCellDeref<'a, T> for &'a UnsafeCell { self.get().read() } } + +trait EnsureAligned { + fn ensure_aligned(self) -> Self; +} + +#[cfg(debug_assertions)] +impl EnsureAligned for *mut T { + #[inline(always)] + fn ensure_aligned(self) -> Self { + let layout = Layout::new::(); + assert!( + self as usize % layout.align() == 0, + "pointer is not aligned. Address {:p} does not have alignemnt {}", + self, + layout.align() + ); + self + } +} + +#[cfg(not(debug_assertions))] +impl EnsureAligned for *mut T { + #[inline(always)] + fn ensure_aligned(self) -> Self { + self + } +} From 59c7835f17fe0cd0738ff47da68eab4cb11dc3c7 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 6 Jan 2023 20:21:34 -0800 Subject: [PATCH 02/13] Use the proper way to ensure the pointer is aligned --- crates/bevy_ptr/src/lib.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index d2a858482381e..c74def78cc33a 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -4,7 +4,7 @@ use core::fmt::{self, Formatter, Pointer}; use core::{ - alloc::Layout, cell::UnsafeCell, marker::PhantomData, mem::ManuallyDrop, num::NonZeroUsize, + cell::UnsafeCell, marker::PhantomData, mem::ManuallyDrop, num::NonZeroUsize, ptr::NonNull, }; @@ -384,12 +384,16 @@ trait EnsureAligned { impl EnsureAligned for *mut T { #[inline(always)] fn ensure_aligned(self) -> Self { - let layout = Layout::new::(); + let align = core::mem::align_of::(); + // 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 % layout.align() == 0, + self as usize & align - 1 == 0, "pointer is not aligned. Address {:p} does not have alignemnt {}", self, - layout.align() + align, ); self } From 4309977bd268abed4cb3986834b8d4d658681a1e Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 6 Jan 2023 20:23:11 -0800 Subject: [PATCH 03/13] Disable the checks for miri builds --- crates/bevy_ptr/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index c74def78cc33a..26564368443d9 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -380,7 +380,9 @@ trait EnsureAligned { fn ensure_aligned(self) -> Self; } -#[cfg(debug_assertions)] +// Disable this for miri runs as it already checks if pointer to reference +// casts are properly aligned. +#[cfg(all(debug_assertions, not(miri)))] impl EnsureAligned for *mut T { #[inline(always)] fn ensure_aligned(self) -> Self { @@ -399,7 +401,7 @@ impl EnsureAligned for *mut T { } } -#[cfg(not(debug_assertions))] +#[cfg(any(not(debug_assertions), miri)] impl EnsureAligned for *mut T { #[inline(always)] fn ensure_aligned(self) -> Self { From 340e2a663d490c2bfca6d05a66c0b59e8c39d0a5 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 6 Jan 2023 21:04:17 -0800 Subject: [PATCH 04/13] Fix syntax error --- crates/bevy_ptr/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 26564368443d9..85d570d3aa294 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -401,7 +401,7 @@ impl EnsureAligned for *mut T { } } -#[cfg(any(not(debug_assertions), miri)] +#[cfg(any(not(debug_assertions), miri))] impl EnsureAligned for *mut T { #[inline(always)] fn ensure_aligned(self) -> Self { From ce6a32cc91168ec30e8cd382b805884acb4d2502 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 6 Jan 2023 21:09:18 -0800 Subject: [PATCH 05/13] Fix formatting --- crates/bevy_ptr/src/lib.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 85d570d3aa294..210edd0d63709 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -4,8 +4,7 @@ use core::fmt::{self, Formatter, Pointer}; use core::{ - cell::UnsafeCell, marker::PhantomData, mem::ManuallyDrop, num::NonZeroUsize, - ptr::NonNull, + cell::UnsafeCell, marker::PhantomData, mem::ManuallyDrop, num::NonZeroUsize, ptr::NonNull, }; /// Type-erased borrow of some unknown type chosen when constructing this type. @@ -387,7 +386,7 @@ impl EnsureAligned for *mut T { #[inline(always)] fn ensure_aligned(self) -> Self { let align = core::mem::align_of::(); - // Implemenation shamelessly borrowed from the currently unstable + // Implemenation shamelessly borrowed from the currently unstable // ptr.is_aligned_to. // // Replace once https://github.com/rust-lang/rust/issues/96284 is stable. From e186cfb41a9d4c67dca86982495ab85badf470b0 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 6 Jan 2023 21:15:54 -0800 Subject: [PATCH 06/13] Fix CI --- crates/bevy_ptr/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 210edd0d63709..fc477bca59dfa 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -391,7 +391,7 @@ impl EnsureAligned for *mut T { // // Replace once https://github.com/rust-lang/rust/issues/96284 is stable. assert!( - self as usize & align - 1 == 0, + self as usize & (align - 1) == 0, "pointer is not aligned. Address {:p} does not have alignemnt {}", self, align, From da74b3ed5d8c4e60f8a3eece4610bc279cde298b Mon Sep 17 00:00:00 2001 From: James Liu Date: Sat, 7 Jan 2023 09:27:58 -0800 Subject: [PATCH 07/13] Apply suggestions from code review Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com> --- crates/bevy_ptr/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index fc477bca59dfa..18ce2fa1e4a33 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -383,7 +383,7 @@ trait EnsureAligned { // casts are properly aligned. #[cfg(all(debug_assertions, not(miri)))] impl EnsureAligned for *mut T { - #[inline(always)] + #[track_caller] fn ensure_aligned(self) -> Self { let align = core::mem::align_of::(); // Implemenation shamelessly borrowed from the currently unstable From 29cb4ad4aa82c6eba61ac7be665fa8c0db6b32bb Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 7 Jan 2023 09:31:04 -0800 Subject: [PATCH 08/13] Add assertion to dangling_with_align --- crates/bevy_ptr/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 18ce2fa1e4a33..00e3601dc3b5c 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -322,6 +322,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 { + 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) } From f4a36f821189e56e105928875d35fc883c0b5d2f Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 7 Jan 2023 09:42:24 -0800 Subject: [PATCH 09/13] Add type name to the assertion --- crates/bevy_ptr/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 00e3601dc3b5c..7614425ae8f0c 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -393,9 +393,10 @@ impl EnsureAligned for *mut T { // 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 {}", + "pointer is not aligned. Address {:p} does not have alignemnt {} for type {}", self, align, + core::any::type_name::(), ); self } From a8e0d637ca4e3d54f35866fac336b722850833b1 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 8 Jan 2023 18:43:57 -0800 Subject: [PATCH 10/13] EnsureAligned -> DebugEnsureAligned --- crates/bevy_ptr/src/lib.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 7614425ae8f0c..968f5dd413569 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -129,7 +129,7 @@ impl<'a> Ptr<'a> { /// Must point to a valid `T` #[inline] pub unsafe fn deref(self) -> &'a T { - &*self.as_ptr().cast::().ensure_aligned() + &*self.as_ptr().cast::().debug_ensure_aligned() } /// Gets the underlying pointer, erasing the associated lifetime. @@ -172,7 +172,7 @@ impl<'a> PtrMut<'a> { /// Must point to a valid `T` #[inline] pub unsafe fn deref_mut(self) -> &'a mut T { - &mut *self.as_ptr().cast::().ensure_aligned() + &mut *self.as_ptr().cast::().debug_ensure_aligned() } /// Gets the underlying pointer, erasing the associated lifetime. @@ -229,7 +229,7 @@ impl<'a> OwningPtr<'a> { /// Must point to a valid `T`. #[inline] pub unsafe fn read(self) -> T { - self.as_ptr().cast::().ensure_aligned().read() + self.as_ptr().cast::().debug_ensure_aligned().read() } /// Consumes the [`OwningPtr`] to drop the underlying data of type `T`. @@ -242,7 +242,10 @@ impl<'a> OwningPtr<'a> { /// Must point to a valid `T`. #[inline] pub unsafe fn drop_as(self) { - self.as_ptr().cast::().ensure_aligned().drop_in_place(); + self.as_ptr() + .cast::() + .debug_ensure_aligned() + .drop_in_place(); } /// Gets the underlying pointer, erasing the associated lifetime. @@ -311,7 +314,7 @@ impl<'a, T> From<&'a [T]> for ThinSlicePtr<'a, T> { let ptr = slice.as_ptr() as *mut T; Self { // SAFETY: a reference can never be null - ptr: unsafe { NonNull::new_unchecked(ptr.ensure_aligned()) }, + ptr: unsafe { NonNull::new_unchecked(ptr.debug_ensure_aligned()) }, #[cfg(debug_assertions)] len: slice.len(), _marker: PhantomData, @@ -376,16 +379,16 @@ impl<'a, T> UnsafeCellDeref<'a, T> for &'a UnsafeCell { } } -trait EnsureAligned { - fn ensure_aligned(self) -> Self; +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 EnsureAligned for *mut T { +impl DebugEnsureAligned for *mut T { #[track_caller] - fn ensure_aligned(self) -> Self { + fn debug_ensure_aligned(self) -> Self { let align = core::mem::align_of::(); // Implemenation shamelessly borrowed from the currently unstable // ptr.is_aligned_to. @@ -403,9 +406,9 @@ impl EnsureAligned for *mut T { } #[cfg(any(not(debug_assertions), miri))] -impl EnsureAligned for *mut T { +impl DebugEnsureAligned for *mut T { #[inline(always)] - fn ensure_aligned(self) -> Self { + fn debug_ensure_aligned(self) -> Self { self } } From 5072f0d63ade52517c74299dbe259bc585167af7 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 8 Jan 2023 18:46:33 -0800 Subject: [PATCH 11/13] Remove panic documentation --- crates/bevy_ptr/src/lib.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 968f5dd413569..04d4a02a84cb8 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -121,10 +121,6 @@ impl<'a> Ptr<'a> { /// Transforms this [`Ptr`] into a `&T` with the same lifetime /// - /// # Panics - /// In debug builds, this function will panic if the pointer is not properly - /// aligned for `T`. - /// /// # Safety /// Must point to a valid `T` #[inline] @@ -164,10 +160,6 @@ impl<'a> PtrMut<'a> { /// Transforms this [`PtrMut`] into a `&mut T` with the same lifetime /// - /// # Panics - /// In debug builds, this function will panic if the pointer is not properly - /// aligned for `T`. - /// /// # Safety /// Must point to a valid `T` #[inline] @@ -221,10 +213,6 @@ impl<'a> OwningPtr<'a> { /// Consumes the [`OwningPtr`] to obtain ownership of the underlying data of type `T`. /// - /// # Panics - /// In debug builds, this function will panic if the pointer is not properly - /// aligned for `T`. - /// /// # Safety /// Must point to a valid `T`. #[inline] @@ -234,10 +222,6 @@ impl<'a> OwningPtr<'a> { /// Consumes the [`OwningPtr`] to drop the underlying data of type `T`. /// - /// # Panics - /// In debug builds, this function will panic if the pointer is not properly - /// aligned for `T`. - /// /// # Safety /// Must point to a valid `T`. #[inline] From 8c34e8e581c5ede6741b673d81d1a879d99f447c Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 8 Jan 2023 18:49:04 -0800 Subject: [PATCH 12/13] Fix BlobVec's initialization for non-ZST types --- crates/bevy_ecs/src/storage/blob_vec.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index c0bfa3302572c..7b9437f384218 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -46,10 +46,11 @@ impl BlobVec { drop: Option)>, 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, @@ -57,7 +58,7 @@ impl BlobVec { } } else { let mut blob_vec = BlobVec { - data: NonNull::dangling(), + data, capacity: 0, len: 0, item_layout, From 9ff927be561adf7a8827e7f75188fd0d79f48865 Mon Sep 17 00:00:00 2001 From: James Liu Date: Wed, 11 Jan 2023 15:05:26 -0500 Subject: [PATCH 13/13] Make the assert only debug. Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com> --- crates/bevy_ptr/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 72f611f4ab56a..d793d62606438 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -381,7 +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 { - assert!(align.is_power_of_two(), "Alignment must be power of two."); + 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) }