From 6f53b2f8f3cdcee424fbf0dda16dab7884791d0d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 18 Aug 2022 09:30:54 +1000 Subject: [PATCH 1/3] Allow `set_len(0)` on an empty `ThinVec`. Currently it crashes. --- src/lib.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 8224259..897f57b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -534,7 +534,13 @@ impl ThinVec { } pub unsafe fn set_len(&mut self, len: usize) { - self.header_mut().set_len(len) + if self.capacity() == 0 { + // A prerequisite of `Vec::set_len` is that `new_len` must be + // less than or equal to capacity(). The same applies here. + assert!(len == 0, "invalid set_len({}) on empty ThinVec", len); + } else { + self.header_mut().set_len(len) + } } pub fn push(&mut self, val: T) { @@ -2749,4 +2755,21 @@ mod std_tests { assert_eq!(padding::>(), 128 - HEADER_SIZE); assert_aligned_head_ptr!(Funky<[*mut usize; 1024]>); } + + #[test] + fn test_set_len() { + let mut vec: ThinVec = thin_vec![]; + unsafe { + vec.set_len(0); // at one point this caused a crash + } + } + + #[test] + #[should_panic(expected = "invalid set_len(1) on empty ThinVec")] + fn test_set_len_invalid() { + let mut vec: ThinVec = thin_vec![]; + unsafe { + vec.set_len(1); + } + } } From 5a6b89669bf9af3aee8eef5a1df5dbf9e8e5e7a6 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 18 Aug 2022 10:03:39 +1000 Subject: [PATCH 2/3] Get precise with singleton checking. - Add `is_singleton`, and change the test in `set_len` to use it, because it's more precise than testing for zero capacity. (Precise in the sense of protecting against the bad case of writing to static memory.) - Remove several "Don't mutate the empty singleton!" checks before `set_len` calls, now that `set_len` is able to handle that itself. - Add a private `set_len_non_singleton`, and use it in places where we know we're not dealing with the singleton (e.g. after calling `reserve(1)`), for ultra-efficiency. --- src/lib.rs | 72 +++++++++++++++++++++++++++--------------------------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 897f57b..38d08bb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -534,7 +534,7 @@ impl ThinVec { } pub unsafe fn set_len(&mut self, len: usize) { - if self.capacity() == 0 { + if self.is_singleton() { // A prerequisite of `Vec::set_len` is that `new_len` must be // less than or equal to capacity(). The same applies here. assert!(len == 0, "invalid set_len({}) on empty ThinVec", len); @@ -543,6 +543,11 @@ impl ThinVec { } } + // For internal use only, when setting the length and it's known to be the non-singleton. + unsafe fn set_len_non_singleton(&mut self, len: usize) { + self.header_mut().set_len(len) + } + pub fn push(&mut self, val: T) { let old_len = self.len(); if old_len == self.capacity() { @@ -550,7 +555,7 @@ impl ThinVec { } unsafe { ptr::write(self.data_raw().add(old_len), val); - self.set_len(old_len + 1); + self.set_len_non_singleton(old_len + 1); } } @@ -561,7 +566,7 @@ impl ThinVec { } unsafe { - self.set_len(old_len - 1); + self.set_len_non_singleton(old_len - 1); Some(ptr::read(self.data_raw().add(old_len - 1))) } } @@ -577,7 +582,7 @@ impl ThinVec { let ptr = self.data_raw(); ptr::copy(ptr.add(idx), ptr.add(idx + 1), old_len - idx); ptr::write(ptr.add(idx), elem); - self.set_len(old_len + 1); + self.set_len_non_singleton(old_len + 1); } } @@ -587,7 +592,7 @@ impl ThinVec { assert!(idx < old_len, "Index out of bounds"); unsafe { - self.set_len(old_len - 1); + self.set_len_non_singleton(old_len - 1); let ptr = self.data_raw(); let val = ptr::read(self.data_raw().add(idx)); ptr::copy(ptr.add(idx + 1), ptr.add(idx), old_len - idx - 1); @@ -603,7 +608,7 @@ impl ThinVec { unsafe { let ptr = self.data_raw(); ptr::swap(ptr.add(idx), ptr.add(old_len - 1)); - self.set_len(old_len - 1); + self.set_len_non_singleton(old_len - 1); ptr::read(ptr.add(old_len - 1)) } } @@ -615,7 +620,7 @@ impl ThinVec { // decrement len before the drop_in_place(), so a panic on Drop // doesn't re-drop the just-failed value. let new_len = self.len() - 1; - self.set_len(new_len); + self.set_len_non_singleton(new_len); ptr::drop_in_place(self.data_raw().add(new_len)); } } @@ -624,11 +629,7 @@ impl ThinVec { pub fn clear(&mut self) { unsafe { ptr::drop_in_place(&mut self[..]); - - // Don't mutate the empty singleton! - if !self.is_empty() { - self.set_len(0); - } + self.set_len(0); // could be the singleton } } @@ -888,14 +889,8 @@ impl ThinVec { ptr::copy_nonoverlapping(self.data_raw().add(at), new_vec.data_raw(), new_vec_len); - // Don't mutate the empty singleton! - if new_vec_len != 0 { - new_vec.set_len(new_vec_len); - } - - if old_len != 0 { - self.set_len(at); - } + new_vec.set_len(new_vec_len); // could be the singleton + self.set_len(at); // could be the singleton new_vec } @@ -925,10 +920,7 @@ impl ThinVec { unsafe { // Set our length to the start bound - // Don't mutate the empty singleton! - if len != 0 { - self.set_len(start); - } + self.set_len(start); // could be the singleton let iter = slice::from_raw_parts_mut(self.data_raw().add(start), end - start).iter_mut(); @@ -986,26 +978,39 @@ impl ThinVec { .add(1) .cast::() .copy_from_nonoverlapping(self.data_raw(), len); - self.set_len(0); + self.set_len_non_singleton(0); } self.ptr = new_header; } } + #[cfg(feature = "gecko-ffi")] + #[inline] + fn is_singleton(&self) -> bool { + unsafe { + self.ptr.as_ptr() as *const Header == &EMPTY_HEADER + } + } + + #[cfg(not(feature = "gecko-ffi"))] + #[inline] + fn is_singleton(&self) -> bool { + self.ptr.as_ptr() as *const Header == &EMPTY_HEADER + } + #[cfg(feature = "gecko-ffi")] #[inline] fn has_allocation(&self) -> bool { unsafe { - self.ptr.as_ptr() as *const Header != &EMPTY_HEADER - && !self.ptr.as_ref().uses_stack_allocated_buffer() + !self.is_singleton() && !self.ptr.as_ref().uses_stack_allocated_buffer() } } #[cfg(not(feature = "gecko-ffi"))] #[inline] fn has_allocation(&self) -> bool { - self.ptr.as_ptr() as *const Header != &EMPTY_HEADER + !self.is_singleton() } } @@ -1332,14 +1337,9 @@ impl DoubleEndedIterator for IntoIter { impl Drop for IntoIter { fn drop(&mut self) { unsafe { - let old_len = self.vec.len(); let mut vec = mem::replace(&mut self.vec, ThinVec::new()); ptr::drop_in_place(&mut vec[self.start..]); - - // Don't mutate the empty singleton! - if old_len != 0 { - vec.set_len(0) - } + vec.set_len(0) // could be the singleton } } } @@ -1373,12 +1373,12 @@ impl<'a, T> Drop for Drain<'a, T> { let vec = &mut *self.vec; // Don't mutate the empty singleton! - if vec.has_allocation() { + if !vec.is_singleton() { let old_len = vec.len(); let start = vec.data_raw().add(old_len); let end = vec.data_raw().add(self.end); ptr::copy(end, start, self.tail); - vec.set_len(old_len + self.tail); + vec.set_len_non_singleton(old_len + self.tail); } } } From e8afb39cd3ce16704774967213024c99ebf837d9 Mon Sep 17 00:00:00 2001 From: Aria Beingessner Date: Thu, 18 Aug 2022 10:51:05 +1000 Subject: [PATCH 3/3] Add more tests. --- src/lib.rs | 205 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 199 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 38d08bb..dc35429 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -988,9 +988,7 @@ impl ThinVec { #[cfg(feature = "gecko-ffi")] #[inline] fn is_singleton(&self) -> bool { - unsafe { - self.ptr.as_ptr() as *const Header == &EMPTY_HEADER - } + unsafe { self.ptr.as_ptr() as *const Header == &EMPTY_HEADER } } #[cfg(not(feature = "gecko-ffi"))] @@ -1002,9 +1000,7 @@ impl ThinVec { #[cfg(feature = "gecko-ffi")] #[inline] fn has_allocation(&self) -> bool { - unsafe { - !self.is_singleton() && !self.ptr.as_ref().uses_stack_allocated_buffer() - } + unsafe { !self.is_singleton() && !self.ptr.as_ref().uses_stack_allocated_buffer() } } #[cfg(not(feature = "gecko-ffi"))] @@ -1540,6 +1536,203 @@ mod tests { for _ in v.drain(MAX_CAP - 1..) {} assert_eq!(v.len(), MAX_CAP - 1); } + + #[test] + fn test_clear() { + let mut v = ThinVec::::new(); + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + + v.clear(); + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + + v.push(1); + v.push(2); + assert_eq!(v.len(), 2); + assert!(v.capacity() >= 2); + assert_eq!(&v[..], &[1, 2]); + + v.clear(); + assert_eq!(v.len(), 0); + assert!(v.capacity() >= 2); + assert_eq!(&v[..], &[]); + + v.push(3); + v.push(4); + assert_eq!(v.len(), 2); + assert!(v.capacity() >= 2); + assert_eq!(&v[..], &[3, 4]); + + v.clear(); + assert_eq!(v.len(), 0); + assert!(v.capacity() >= 2); + assert_eq!(&v[..], &[]); + + v.clear(); + assert_eq!(v.len(), 0); + assert!(v.capacity() >= 2); + assert_eq!(&v[..], &[]); + } + + #[test] + fn test_empty_singleton_torture() { + { + let mut v = ThinVec::::new(); + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert!(v.is_empty()); + assert_eq!(&v[..], &[]); + assert_eq!(&mut v[..], &mut []); + + assert_eq!(v.pop(), None); + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let v = ThinVec::::new(); + assert_eq!(v.into_iter().count(), 0); + + let v = ThinVec::::new(); + for _ in v.into_iter() { + unreachable!(); + } + } + + { + let mut v = ThinVec::::new(); + assert_eq!(v.drain(..).len(), 0); + + for _ in v.drain(..) { + unreachable!() + } + + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let mut v = ThinVec::::new(); + v.truncate(1); + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + + v.truncate(0); + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let mut v = ThinVec::::new(); + v.shrink_to_fit(); + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let mut v = ThinVec::::new(); + let new = v.split_off(0); + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + + assert_eq!(new.len(), 0); + assert_eq!(new.capacity(), 0); + assert_eq!(&new[..], &[]); + } + + { + let mut v = ThinVec::::new(); + let mut other = ThinVec::::new(); + v.append(&mut other); + + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + + assert_eq!(other.len(), 0); + assert_eq!(other.capacity(), 0); + assert_eq!(&other[..], &[]); + } + + { + let mut v = ThinVec::::new(); + v.reserve(0); + + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let mut v = ThinVec::::new(); + v.reserve_exact(0); + + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let mut v = ThinVec::::new(); + v.reserve(0); + + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let v = ThinVec::::with_capacity(0); + + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let v = ThinVec::::default(); + + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let mut v = ThinVec::::new(); + v.retain(|_| unreachable!()); + + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let mut v = ThinVec::::new(); + v.dedup_by_key(|x| *x); + + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + + { + let mut v = ThinVec::::new(); + v.dedup_by(|_, _| unreachable!()); + + assert_eq!(v.len(), 0); + assert_eq!(v.capacity(), 0); + assert_eq!(&v[..], &[]); + } + } } #[cfg(test)]