Bug fixes for StrV[Ref]

glib/src/collections/strv.rs

@@ -712,7 +712,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,17 +772,19 @@ impl StrV {
     #[inline]
     pub fn extend_from_slice<S: AsRef<str>>(&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());
         }
 
         unsafe {
             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();
+            }
         }
     }
 
@@ -1464,6 +1466,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;
@@ -1781,4 +1787,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<str> 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<str> 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"][..]);
+    }
 }