Fix integer overflow in BlobVec::push for ZST (#10799)

`reserve_exact` is no-op for ZST because `self.item_layout.size() > 0`
is always `false`.


daa8bf20df/crates/bevy_ecs/src/storage/blob_vec.rs (L112-L120)

Then in `push` we just increase `.len` ignoring integer overflow.


daa8bf20df/crates/bevy_ecs/src/storage/blob_vec.rs (L232-L237)
This commit is contained in:
Stepan Koltsov 2024-01-04 21:32:05 +00:00 committed by GitHub
parent cc2a77b5c5
commit cf70f53227
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23

View file

@ -61,6 +61,8 @@ impl BlobVec {
if item_layout.size() == 0 { if item_layout.size() == 0 {
BlobVec { BlobVec {
data, data,
// ZST `BlobVec` max size is `usize::MAX`, and `reserve_exact` for ZST assumes
// the capacity is always `usize::MAX` and panics if it overflows.
capacity: usize::MAX, capacity: usize::MAX,
len: 0, len: 0,
item_layout, item_layout,
@ -109,20 +111,29 @@ impl BlobVec {
/// ///
/// Note that the allocator may give the collection more space than it requests. Therefore, capacity can not be relied upon /// Note that the allocator may give the collection more space than it requests. Therefore, capacity can not be relied upon
/// to be precisely minimal. /// to be precisely minimal.
///
/// # Panics
///
/// Panics if new capacity overflows `usize`.
pub fn reserve_exact(&mut self, additional: usize) { pub fn reserve_exact(&mut self, additional: usize) {
let available_space = self.capacity - self.len; let available_space = self.capacity - self.len;
if available_space < additional && self.item_layout.size() > 0 { if available_space < additional {
// SAFETY: `available_space < additional`, so `additional - available_space > 0` // SAFETY: `available_space < additional`, so `additional - available_space > 0`
let increment = unsafe { NonZeroUsize::new_unchecked(additional - available_space) }; let increment = unsafe { NonZeroUsize::new_unchecked(additional - available_space) };
// SAFETY: not called for ZSTs self.grow_exact(increment);
unsafe { self.grow_exact(increment) };
} }
} }
// SAFETY: must not be called for a ZST item layout /// Grows the capacity by `increment` elements.
#[warn(unsafe_op_in_unsafe_fn)] // to allow unsafe blocks in unsafe fn ///
unsafe fn grow_exact(&mut self, increment: NonZeroUsize) { /// # Panics
debug_assert!(self.item_layout.size() != 0); ///
/// Panics if the new capacity overflows `usize`.
/// For ZST it panics unconditionally because ZST `BlobVec` capacity
/// is initialized to `usize::MAX` and always stays that way.
fn grow_exact(&mut self, increment: NonZeroUsize) {
// If we got here with ZST, we requested total capacity > usize::MAX.
assert!(self.item_layout.size() != 0, "ZST capacity overflow");
let new_capacity = self.capacity + increment.get(); let new_capacity = self.capacity + increment.get();
let new_layout = let new_layout =
@ -609,6 +620,30 @@ mod tests {
let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) }; let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) };
} }
#[test]
#[should_panic(expected = "ZST capacity overflow")]
fn blob_vec_zst_size_overflow() {
// SAFETY: no drop is correct drop for `()`.
let mut blob_vec = unsafe { BlobVec::new(Layout::new::<()>(), None, 0) };
assert_eq!(usize::MAX, blob_vec.capacity(), "Self-check");
// SAFETY: Because `()` is a ZST trivial drop type, and because `BlobVec` capacity
// is always `usize::MAX` for ZSTs, we can arbitrarily set the length
// and still be sound.
unsafe {
blob_vec.set_len(usize::MAX);
}
// SAFETY: `BlobVec` was initialized for `()`, so it is safe to push `()` to it.
unsafe {
OwningPtr::make((), |ptr| {
// This should panic because len is usize::MAX, remaining capacity is 0.
blob_vec.push(ptr);
});
}
}
#[test] #[test]
fn aligned_zst() { fn aligned_zst() {
// NOTE: This test is explicitly for uncovering potential UB with miri. // NOTE: This test is explicitly for uncovering potential UB with miri.