Fix soundness issue with NSArray::objects_in_range

madsmtm · Dec 23, 2022 · 80bdfcb · 80bdfcb
1 parent 652ad14
commit 80bdfcb
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 5 deletions.
diff --git a/crates/icrate/CHANGELOG.md b/crates/icrate/CHANGELOG.md
@@ -68,6 +68,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 * **BREAKING**: Changed `NSDictionary` to be `Shared` by default.
 * **BREAKING** (TEMPORARY): Renamed `NSEnumerator`, `NSFastEnumeration` and
   `NSFastEnumerator` until the story around them are properly figured out.
+* **BREAKING**: Make `NSArray::objects_in_range` return an `Option` (it was
+  unsound before).
 
 ### Fixed
 * Fixed `NSZone` not being `#[repr(C)]`.

diff --git a/crates/icrate/src/Foundation/additions/array.rs b/crates/icrate/src/Foundation/additions/array.rs
@@ -132,7 +132,7 @@ extern_methods!(
             }
         }
 
-        pub fn objects_in_range(&self, range: Range<usize>) -> Vec<&T> {
+        unsafe fn objects_in_range_unchecked(&self, range: Range<usize>) -> Vec<&T> {
             let range = NSRange::from(range);
             let mut vec: Vec<NonNull<T>> = Vec::with_capacity(range.length);
             unsafe {
@@ -142,8 +142,17 @@ extern_methods!(
             }
         }
 
+        pub fn objects_in_range(&self, range: Range<usize>) -> Option<Vec<&T>> {
+            if range.end > self.len() {
+                return None;
+            }
+            // SAFETY: Just checked that the range is in bounds
+            Some(unsafe { self.objects_in_range_unchecked(range) })
+        }
+
         pub fn to_vec(&self) -> Vec<&T> {
-            self.objects_in_range(0..self.len())
+            // SAFETY: The range is know to be in bounds
+            unsafe { self.objects_in_range_unchecked(0..self.len()) }
         }
 
         // TODO: Take Id<Self, Self::ItemOwnership> ?
@@ -426,15 +435,15 @@ mod tests {
     fn test_objects_in_range() {
         let array = sample_array(4);
 
-        let middle_objs = array.objects_in_range(1..3);
+        let middle_objs = array.objects_in_range(1..3).unwrap();
         assert_eq!(middle_objs.len(), 2);
         assert_eq!(middle_objs[0], array.get(1).unwrap());
         assert_eq!(middle_objs[1], array.get(2).unwrap());
 
-        let empty_objs = array.objects_in_range(1..1);
+        let empty_objs = array.objects_in_range(1..1).unwrap();
         assert!(empty_objs.is_empty());
 
-        let all_objs = array.objects_in_range(0..4);
+        let all_objs = array.objects_in_range(0..4).unwrap();
         assert_eq!(all_objs.len(), 4);
     }