diff --git a/glib/src/collections/ptr_slice.rs b/glib/src/collections/ptr_slice.rs index 32320555dea0..3388c0a1e931 100644 --- a/glib/src/collections/ptr_slice.rs +++ b/glib/src/collections/ptr_slice.rs @@ -713,7 +713,7 @@ impl PtrSlice { #[allow(clippy::int_plus_one)] pub fn reserve(&mut self, additional: usize) { // Nothing new to reserve as there's still enough space - if self.len + additional + 1 <= self.capacity { + if additional < self.capacity - self.len { return; } @@ -788,7 +788,7 @@ impl PtrSlice { #[inline] pub fn extend_from_slice(&mut self, other: &[T]) { // Nothing new to reserve as there's still enough space - if self.len + other.len() + 1 > self.capacity { + if other.len() >= self.capacity - self.len { self.reserve(other.len()); } @@ -796,12 +796,14 @@ impl PtrSlice { for item in other { ptr::write(self.ptr.as_ptr().add(self.len) as *mut T, item.clone()); self.len += 1; - } - ptr::write( - self.ptr.as_ptr().add(self.len), - Ptr::from(ptr::null_mut::<::GlibType>()), - ); + // Add null terminator on every iteration because `clone` + // may panic + ptr::write( + self.ptr.as_ptr().add(self.len), + Ptr::from(ptr::null_mut::<::GlibType>()), + ); + } } } @@ -813,7 +815,7 @@ impl PtrSlice { assert!(index <= self.len); // Nothing new to reserve as there's still enough space - if self.len + 1 + 1 > self.capacity { + if 1 >= self.capacity - self.len { self.reserve(1); } @@ -840,7 +842,7 @@ impl PtrSlice { #[inline] pub fn push(&mut self, item: T) { // Nothing new to reserve as there's still enough space - if self.len + 1 + 1 > self.capacity { + if 1 >= self.capacity - self.len { self.reserve(1); } diff --git a/glib/src/collections/slice.rs b/glib/src/collections/slice.rs index 8dc771c9ef94..93ec467c3545 100644 --- a/glib/src/collections/slice.rs +++ b/glib/src/collections/slice.rs @@ -614,7 +614,7 @@ impl Slice { /// Reserves at least this much additional capacity. pub fn reserve(&mut self, additional: usize) { // Nothing new to reserve as there's still enough space - if self.len + additional <= self.capacity { + if additional <= self.capacity - self.len { return; } @@ -685,7 +685,7 @@ impl Slice { #[inline] pub fn extend_from_slice(&mut self, other: &[T]) { // Nothing new to reserve as there's still enough space - if self.len + other.len() > self.capacity { + if other.len() > self.capacity - self.len { self.reserve(other.len()); } @@ -706,7 +706,7 @@ impl Slice { assert!(index <= self.len); // Nothing new to reserve as there's still enough space - if self.len + 1 > self.capacity { + if 1 > self.capacity - self.len { self.reserve(1); } @@ -729,7 +729,7 @@ impl Slice { #[allow(clippy::int_plus_one)] pub fn push(&mut self, item: T) { // Nothing new to reserve as there's still enough space - if self.len + 1 > self.capacity { + if 1 > self.capacity - self.len { self.reserve(1); } diff --git a/glib/src/collections/strv.rs b/glib/src/collections/strv.rs index 9451c050edeb..3789840fd02b 100644 --- a/glib/src/collections/strv.rs +++ b/glib/src/collections/strv.rs @@ -65,6 +65,10 @@ impl std::hash::Hash for StrV { impl PartialEq<[&'_ str]> for StrV { fn eq(&self, other: &[&'_ str]) -> bool { + if self.len() != other.len() { + return false; + } + for (a, b) in Iterator::zip(self.iter(), other.iter()) { if a != b { return false; @@ -712,7 +716,7 @@ impl StrV { #[allow(clippy::int_plus_one)] pub fn reserve(&mut self, additional: usize) { // Nothing new to reserve as there's still enough space - if self.len + additional + 1 <= self.capacity { + if additional < self.capacity - self.len { return; } @@ -772,7 +776,7 @@ impl StrV { #[inline] pub fn extend_from_slice>(&mut self, other: &[S]) { // Nothing new to reserve as there's still enough space - if self.len + other.len() + 1 > self.capacity { + if other.len() >= self.capacity - self.len { self.reserve(other.len()); } @@ -780,9 +784,11 @@ impl StrV { for item in other { *self.ptr.as_ptr().add(self.len) = GString::from(item.as_ref()).into_glib_ptr(); self.len += 1; - } - *self.ptr.as_ptr().add(self.len) = ptr::null_mut(); + // Add null terminator on every iteration because `as_ref` + // may panic + *self.ptr.as_ptr().add(self.len) = ptr::null_mut(); + } } } @@ -794,7 +800,7 @@ impl StrV { assert!(index <= self.len); // Nothing new to reserve as there's still enough space - if self.len + 1 + 1 > self.capacity { + if 1 >= self.capacity - self.len { self.reserve(1); } @@ -818,7 +824,7 @@ impl StrV { #[inline] pub fn push(&mut self, item: GString) { // Nothing new to reserve as there's still enough space - if self.len + 1 + 1 > self.capacity { + if 1 >= self.capacity - self.len { self.reserve(1); } @@ -1464,6 +1470,10 @@ impl std::hash::Hash for StrVRef { impl PartialEq<[&'_ str]> for StrVRef { fn eq(&self, other: &[&'_ str]) -> bool { + if self.len() != other.len() { + return false; + } + for (a, b) in Iterator::zip(self.iter(), other.iter()) { if a != b { return false; @@ -1598,6 +1608,7 @@ mod test { StrV::from_glib_full_num(ptr, 4, false) }; + assert_eq!(items.len(), slice.len()); for (a, b) in Iterator::zip(items.iter(), slice.iter()) { assert_eq!(a, b); } @@ -1622,6 +1633,7 @@ mod test { StrV::from_glib_container_num(ptr, 4, false) }; + assert_eq!(items.len(), slice.len()); for (a, b) in Iterator::zip(items.iter(), slice.iter()) { assert_eq!(a, b); } @@ -1648,6 +1660,7 @@ mod test { res }; + assert_eq!(items.len(), slice.len()); for (a, b) in Iterator::zip(items.iter(), slice.iter()) { assert_eq!(a, b); } @@ -1781,4 +1794,80 @@ mod test { assert!(strv.contains("str2")); assert!(!strv.contains("str4")); } + + #[test] + #[should_panic] + fn test_reserve_overflow() { + let mut strv = StrV::from(&[crate::gstr!("foo"); 3][..]); + + // An old implementation of `reserve` used the condition `self.len + + // additional + 1 <= self.capacity`, which was prone to overflow + strv.reserve(usize::MAX - 3); + } + + #[test] + #[should_panic] + fn test_extend_from_slice_overflow() { + // We need a zero-sized type because only a slice of ZST can legally + // contain up to `usize::MAX` elements. + #[derive(Clone, Copy)] + struct ImplicitStr; + + impl AsRef for ImplicitStr { + fn as_ref(&self) -> &str { + "" + } + } + + let mut strv = StrV::from(&[crate::gstr!(""); 3][..]); + + // An old implementation of `extend_from_slice` used the condition + // `self.len + other.len() + 1 <= self.capacity`, which was prone to + // overflow + strv.extend_from_slice(&[ImplicitStr; usize::MAX - 3]); + } + + #[test] + fn test_extend_from_slice_panic_safe() { + struct MayPanic(bool); + + impl AsRef for MayPanic { + fn as_ref(&self) -> &str { + if self.0 { + panic!("panicking as per request"); + } else { + "" + } + } + } + + let mut strv = StrV::from(&[crate::gstr!(""); 3][..]); + strv.clear(); + + // Write one element and panic while getting the second element + _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + strv.extend_from_slice(&[MayPanic(false), MayPanic(true)]); + })); + + // Check that it contains up to one element is null-terminated + assert!(strv.len() <= 1); + unsafe { + for i in 0..strv.len() { + assert!(!(*strv.as_ptr().add(i)).is_null()); + } + assert!((*strv.as_ptr().add(strv.len())).is_null()); + } + } + + #[test] + fn test_strv_ref_eq_str_slice() { + let strv = StrV::from(&[crate::gstr!("a")][..]); + let strv_ref: &StrVRef = strv.as_ref(); + + // Test `impl PartialEq<[&'_ str]> for StrVRef` + assert_eq!(strv_ref, &["a"][..]); + assert_ne!(strv_ref, &[][..]); + assert_ne!(strv_ref, &["a", "b"][..]); + assert_ne!(strv_ref, &["b"][..]); + } }