Skip to content

[Merged by Bors] - Add safety comments to usages of byte_add (Ptr, PtrMut, OwningPtr) #7214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 31 additions & 11 deletions crates/bevy_ecs/src/storage/blob_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ impl BlobVec {
#[must_use = "The returned pointer should be used to dropped the removed element"]
pub unsafe fn swap_remove_and_forget_unchecked(&mut self, index: usize) -> OwningPtr<'_> {
debug_assert!(index < self.len());
// Since `index` must be strictly less than `self.len` and `index` is at least zero,
// `self.len` must be at least one. Thus, this cannot underflow.
let new_len = self.len - 1;
let size = self.item_layout.size();
if index != new_len {
Expand All @@ -223,6 +225,10 @@ impl BlobVec {
}
self.len = new_len;
// Cannot use get_unchecked here as this is technically out of bounds after changing len.
// SAFETY:
// - `new_len` is less than the old len, so it must fit in this vector's allocation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not account for the case of underflow. Actually we don't mention that this shouldn't be called on an empty BlobVec in the safety invariants at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the caller's responsibility to ensure that index is less than self.len().

if self.len() is 0 then it is not possible to pass in an index that is less than self.len()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah OK fair. Still think it's worth mentioning something along these lines in this comment though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment about this to the declaration of new_len.

// - `size` is a multiple of the erased type's alignment,
// so adding a multiple of `size` will preserve alignment.
self.get_ptr_mut().byte_add(new_len * size).promote()
}

Expand Down Expand Up @@ -264,16 +270,27 @@ impl BlobVec {
#[inline]
pub unsafe fn get_unchecked(&self, index: usize) -> Ptr<'_> {
debug_assert!(index < self.len());
self.get_ptr().byte_add(index * self.item_layout.size())
let size = self.item_layout.size();
// SAFETY:
// - The caller ensures that `index` fits in this vector,
// so this operation will not overflow the original allocation.
// - `size` is a multiple of the erased type's alignment,
// so adding a multiple of `size` will preserve alignment.
self.get_ptr().byte_add(index * size)
}

/// # Safety
/// It is the caller's responsibility to ensure that `index` is < self.len()
#[inline]
pub unsafe fn get_unchecked_mut(&mut self, index: usize) -> PtrMut<'_> {
debug_assert!(index < self.len());
let layout_size = self.item_layout.size();
self.get_ptr_mut().byte_add(index * layout_size)
let size = self.item_layout.size();
// SAFETY:
// - The caller ensures that `index` fits in this vector,
// so this operation will not overflow the original allocation.
// - `size` is a multiple of the erased type's alignment,
// so adding a multiple of `size` will preserve alignment.
self.get_ptr_mut().byte_add(index * size)
}

/// Gets a [`Ptr`] to the start of the vec
Expand Down Expand Up @@ -305,15 +322,18 @@ impl BlobVec {
// accidentally drop elements twice in the event of a drop impl panicking.
self.len = 0;
if let Some(drop) = self.drop {
let layout_size = self.item_layout.size();
let size = self.item_layout.size();
for i in 0..len {
// SAFETY: `i * layout_size` is inbounds for the allocation, and the item is left unreachable so it can be safely promoted to an `OwningPtr`
unsafe {
// NOTE: this doesn't use self.get_unchecked(i) because the debug_assert on index
// will panic here due to self.len being set to 0
let ptr = self.get_ptr_mut().byte_add(i * layout_size).promote();
(drop)(ptr);
}
// SAFETY:
// * 0 <= `i` < `len`, so `i * size` must be in bounds for the allocation.
// * `size` is a multiple of the erased type's alignment,
// so adding a multiple of `size` will preserve alignment.
// * The item is left unreachable so it can be safely promoted to an `OwningPtr`.
// NOTE: `self.get_unchecked_mut(i)` cannot be used here, since the `debug_assert`
// would panic due to `self.len` being set to 0.
let item = unsafe { self.get_ptr_mut().byte_add(i * size).promote() };
// SAFETY: `item` was obtained from this `BlobVec`, so its underlying type must match `drop`.
unsafe { drop(item) };
}
}
}
Expand Down