From 957f0b089410e60f32a2a58aaf167d75c3ab7dae Mon Sep 17 00:00:00 2001 From: Aria Beingessner Date: Sun, 19 Feb 2023 19:12:18 -0500 Subject: [PATCH 1/2] Handle overflows in capacities more I was mentally applying the rule that capacity can never exceed isize::MAX as a precondition BUT this is the code necessary for enforcing that! In fixing this I broke the fact that this code was subtly relying on that overflow to allow usize::MAX ZSTs to be allocated (by only allocating space for the header). ZSTs previously actually worked fine, as the garbage overflowed value was always wiped out by multiplying by 0 to get the array size. Now we need to handle it more explicitly. --- src/lib.rs | 44 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 2497bcd..b684899 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -147,6 +147,7 @@ use std::alloc::*; use std::borrow::*; use std::cmp::*; use std::convert::TryFrom; +use std::convert::TryInto; use std::hash::*; use std::iter::FromIterator; use std::marker::PhantomData; @@ -345,12 +346,18 @@ fn alloc_size(cap: usize) -> usize { // // We turn everything into isizes here so that we can catch isize::MAX overflow, // we never want to allow allocations larger than that! - let cap = cap as isize; let header_size = mem::size_of::
() as isize; - let elem_size = mem::size_of::() as isize; let padding = padding::() as isize; - let data_size = elem_size.checked_mul(cap).expect("capacity overflow"); + let data_size = if mem::size_of::() == 0 { + // If we're allocating an array for ZSTs we need a header/padding but no actual + // space for items, so we don't care about the capacity that was requested! + 0 + } else { + let cap: isize = cap.try_into().expect("capacity overflow"); + let elem_size = mem::size_of::() as isize; + elem_size.checked_mul(cap).expect("capacity overflow") + }; let final_size = data_size .checked_add(header_size + padding) @@ -4242,4 +4249,35 @@ mod std_tests { vec.set_len(1); } } + + #[test] + #[should_panic(expected = "capacity overflow")] + fn test_capacity_overflow_header_too_big() { + let vec: ThinVec = ThinVec::with_capacity(isize::MAX as usize - 2); + assert!(vec.capacity() > 0); + } + #[test] + #[should_panic(expected = "capacity overflow")] + fn test_capacity_overflow_cap_too_big() { + let vec: ThinVec = ThinVec::with_capacity(isize::MAX as usize + 1); + assert!(vec.capacity() > 0); + } + #[test] + #[should_panic(expected = "capacity overflow")] + fn test_capacity_overflow_size_mul1() { + let vec: ThinVec = ThinVec::with_capacity(isize::MAX as usize + 1); + assert!(vec.capacity() > 0); + } + #[test] + #[should_panic(expected = "capacity overflow")] + fn test_capacity_overflow_size_mul2() { + let vec: ThinVec = ThinVec::with_capacity(isize::MAX as usize / 2 + 1); + assert!(vec.capacity() > 0); + } + #[test] + #[should_panic(expected = "capacity overflow")] + fn test_capacity_overflow_cap_really_isnt_isize() { + let vec: ThinVec = ThinVec::with_capacity(isize::MAX as usize); + assert!(vec.capacity() > 0); + } } From d88ed827e980f6e9a2a8ef14c7f46ef28c6ac97d Mon Sep 17 00:00:00 2001 From: Aria Beingessner Date: Sun, 19 Feb 2023 19:17:44 -0500 Subject: [PATCH 2/2] properly explain why the unsafe is needed and shush the warning in tests --- src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index b684899..ea24aed 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1541,7 +1541,12 @@ impl ThinVec { #[cfg(feature = "gecko-ffi")] #[inline] + #[allow(unused_unsafe)] fn is_singleton(&self) -> bool { + // NOTE: the tests will complain that this "unsafe" isn't needed, but it *IS*! + // In production this refers to an *extern static* which *is* unsafe to reference. + // In tests this refers to a local static because we don't have Firefox's codebase + // providing the symbol! unsafe { self.ptr.as_ptr() as *const Header == &EMPTY_HEADER } }