Bug fixes for StrV[Ref]

glib/src/collections/ptr_slice.rs

@@ -713,7 +713,7 @@ impl<T: TransparentPtrType> PtrSlice<T> {
     #[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,20 +788,22 @@ impl<T: TransparentPtrType> PtrSlice<T> {
     #[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());
         }
 
         unsafe {
             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::<<T as GlibPtrDefault>::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::<<T as GlibPtrDefault>::GlibType>()),
+                );
+            }
         }
     }
 
@@ -813,7 +815,7 @@ impl<T: TransparentPtrType> PtrSlice<T> {
         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<T: TransparentPtrType> PtrSlice<T> {
     #[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);
         }
 

glib/src/collections/slice.rs

@@ -614,7 +614,7 @@ impl<T: TransparentType> Slice<T> {
     /// 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<T: TransparentType> Slice<T> {
     #[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<T: TransparentType> Slice<T> {
         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<T: TransparentType> Slice<T> {
     #[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);
         }
 

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,17 +776,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();
+            }
         }
     }
 
@@ -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<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"][..]);
+    }
 }