Merge pull request #163 from madsmtm/soundness-fixes

Small soundness fixes and code cleanup

Author: madsmtm

block2/src/global.rs

@@ -83,8 +83,10 @@ where
     type Target = Block<A, R>;
 
     fn deref(&self) -> &Self::Target {
+        let ptr: *const Self = self;
+        let ptr: *const Block<A, R> = ptr.cast();
         // TODO: SAFETY
-        unsafe { &*(self as *const Self as *const Block<A, R>) }
+        unsafe { ptr.as_ref().unwrap_unchecked() }
     }
 }
 

block2/src/lib.rs

@@ -468,7 +468,10 @@ impl<A, R, F> Deref for ConcreteBlock<A, R, F> {
     type Target = Block<A, R>;
 
     fn deref(&self) -> &Self::Target {
-        unsafe { &*(self as *const Self as *const Block<A, R>) }
+        let ptr: *const Self = self;
+        let ptr: *const Block<A, R> = ptr.cast();
+        // TODO: SAFETY
+        unsafe { ptr.as_ref().unwrap_unchecked() }
     }
 }
 

objc2-foundation/src/array.rs

@@ -57,14 +57,13 @@ unsafe fn from_refs<T: Message + ?Sized>(cls: &Class, refs: &[&T]) -> *mut Objec
     }
 }
 
-impl<T: Message, O: Ownership> NSArray<T, O> {
+impl<T: Message> NSArray<T, Shared> {
     unsafe_def_fn! {
-        /// The `NSArray` itself (length and number of items) is always immutable,
-        /// but we would like to know when we're the only owner of the array, to
-        /// allow mutation of the array's items.
-        pub fn new -> O;
+        pub fn new -> Shared;
     }
+}
 
+impl<T: Message, O: Ownership> NSArray<T, O> {
     #[doc(alias = "count")]
     pub fn len(&self) -> usize {
         unsafe { msg_send![self, count] }
@@ -103,6 +102,9 @@ impl<T: Message, O: Ownership> NSArray<T, O> {
         }
     }
 
+    // The `NSArray` itself (length and number of items) is always immutable,
+    // but we would like to know when we're the only owner of the array, to
+    // allow mutation of the array's items.
     pub fn from_vec(vec: Vec<Id<T, O>>) -> Id<Self, O> {
         unsafe { Id::new(from_refs(Self::class(), vec.as_slice_ref()).cast()).unwrap() }
     }
@@ -204,8 +206,8 @@ impl<T: Message, O: Ownership> Index<usize> for NSArray<T, O> {
     }
 }
 
-impl<T: Message, O: Ownership> DefaultId for NSArray<T, O> {
-    type Ownership = O;
+impl<T: Message> DefaultId for NSArray<T, Shared> {
+    type Ownership = Shared;
 
     #[inline]
     fn default_id() -> Id<Self, Self::Ownership> {
@@ -308,7 +310,7 @@ impl<T: Message, O: Ownership> NSMutableArray<T, O> {
             // Bring back a reference to the closure.
             // Guaranteed to be unique, we gave `sortUsingFunction` unique is
             // ownership, and that method only runs one function at a time.
-            let closure: &mut F = unsafe { &mut *(context as *mut F) };
+            let closure: &mut F = unsafe { (context as *mut F).as_mut().unwrap_unchecked() };
 
             NSComparisonResult::from((*closure)(obj1, obj2))
         }
@@ -407,9 +409,15 @@ mod tests {
         unsafe { msg_send![obj, retainCount] }
     }
 
+    #[test]
+    fn test_two_empty() {
+        let _empty_array1 = NSArray::<NSObject, _>::new();
+        let _empty_array2 = NSArray::<NSObject, _>::new();
+    }
+
     #[test]
     fn test_len() {
-        let empty_array = NSArray::<NSObject, Owned>::new();
+        let empty_array = NSArray::<NSObject, _>::new();
         assert_eq!(empty_array.len(), 0);
 
         let array = sample_array(4);
@@ -450,7 +458,7 @@ mod tests {
         assert_eq!(array.first(), array.get(0));
         assert_eq!(array.last(), array.get(3));
 
-        let empty_array = <NSArray<NSObject, Owned>>::new();
+        let empty_array = <NSArray<NSObject, Shared>>::new();
         assert!(empty_array.first().is_none());
         assert!(empty_array.last().is_none());
     }

objc2-foundation/src/data.rs

@@ -53,11 +53,12 @@ impl NSData {
 
     pub fn bytes(&self) -> &[u8] {
         let ptr: *const c_void = unsafe { msg_send![self, bytes] };
+        let ptr: *const u8 = ptr.cast();
         // The bytes pointer may be null for length zero
         if ptr.is_null() {
             &[]
         } else {
-            unsafe { slice::from_raw_parts(ptr as *const u8, self.len()) }
+            unsafe { slice::from_raw_parts(ptr, self.len()) }
         }
     }
 
@@ -172,11 +173,12 @@ impl NSMutableData {
     #[doc(alias = "mutableBytes")]
     pub fn bytes_mut(&mut self) -> &mut [u8] {
         let ptr = self.raw_bytes_mut();
+        let ptr: *mut u8 = ptr.cast();
         // The bytes pointer may be null for length zero
         if ptr.is_null() {
             &mut []
         } else {
-            unsafe { slice::from_raw_parts_mut(ptr as *mut u8, self.len()) }
+            unsafe { slice::from_raw_parts_mut(ptr, self.len()) }
         }
     }
 
@@ -188,7 +190,7 @@ impl NSMutableData {
 
     #[doc(alias = "appendBytes:length:")]
     pub fn extend_from_slice(&mut self, bytes: &[u8]) {
-        let bytes_ptr = bytes.as_ptr() as *const c_void;
+        let bytes_ptr: *const c_void = bytes.as_ptr().cast();
         unsafe { msg_send![self, appendBytes: bytes_ptr, length: bytes.len()] }
     }
 
@@ -201,7 +203,7 @@ impl NSMutableData {
         let range = NSRange::from(range);
         // No need to verify the length of the range here,
         // `replaceBytesInRange:` just zero-fills if out of bounds.
-        let ptr = bytes.as_ptr() as *const c_void;
+        let ptr: *const c_void = bytes.as_ptr().cast();
         unsafe {
             msg_send![
                 self,
@@ -312,7 +314,7 @@ impl DefaultId for NSMutableData {
 }
 
 unsafe fn data_with_bytes(cls: &Class, bytes: &[u8]) -> *mut Object {
-    let bytes_ptr = bytes.as_ptr() as *const c_void;
+    let bytes_ptr: *const c_void = bytes.as_ptr().cast();
     unsafe {
         let obj: *mut Object = msg_send![cls, alloc];
         msg_send![
@@ -333,18 +335,19 @@ unsafe fn data_from_vec(cls: &Class, bytes: Vec<u8>) -> *mut Object {
 
     let dealloc = ConcreteBlock::new(move |bytes: *mut c_void, len: usize| unsafe {
         // Recreate the Vec and let it drop
-        let _ = Vec::from_raw_parts(bytes as *mut u8, len, capacity);
+        let _ = Vec::<u8>::from_raw_parts(bytes.cast(), len, capacity);
     });
     let dealloc = dealloc.copy();
     let dealloc: &Block<(*mut c_void, usize), ()> = &dealloc;
 
     let mut bytes = ManuallyDrop::new(bytes);
+    let bytes_ptr: *mut c_void = bytes.as_mut_ptr().cast();
 
     unsafe {
         let obj: *mut Object = msg_send![cls, alloc];
         msg_send![
             obj,
-            initWithBytesNoCopy: bytes.as_mut_ptr() as *mut c_void,
+            initWithBytesNoCopy: bytes_ptr,
             length: bytes.len(),
             deallocator: dealloc,
         ]

objc2-foundation/src/enumerator.rs

@@ -159,7 +159,7 @@ impl<'a, C: NSFastEnumeration + ?Sized> Iterator for NSFastEnumerator<'a, C> {
             unsafe {
                 let obj = *self.ptr;
                 self.ptr = self.ptr.offset(1);
-                Some(&*obj)
+                Some(obj.as_ref().unwrap_unchecked())
             }
         }
     }
@@ -172,25 +172,25 @@ mod tests {
 
     #[test]
     fn test_enumerator() {
-        let vec = (0u32..4).map(NSValue::new).collect();
+        let vec = (0usize..4).map(NSValue::new).collect();
         let array = NSArray::from_vec(vec);
 
         let enumerator = array.iter();
         assert_eq!(enumerator.count(), 4);
 
         let enumerator = array.iter();
-        assert!(enumerator.enumerate().all(|(i, obj)| obj.get() == i as u32));
+        assert!(enumerator.enumerate().all(|(i, obj)| obj.get() == i));
     }
 
     #[test]
     fn test_fast_enumerator() {
-        let vec = (0u32..4).map(NSValue::new).collect();
+        let vec = (0usize..4).map(NSValue::new).collect();
         let array = NSArray::from_vec(vec);
 
         let enumerator = array.iter_fast();
         assert_eq!(enumerator.count(), 4);
 
         let enumerator = array.iter_fast();
-        assert!(enumerator.enumerate().all(|(i, obj)| obj.get() == i as u32));
+        assert!(enumerator.enumerate().all(|(i, obj)| obj.get() == i));
     }
 }

objc2-foundation/src/macros.rs

@@ -178,7 +178,7 @@ macro_rules! object {
         impl<$($t: ::core::cmp::PartialEq $(+ $b)?),*> ::core::cmp::PartialEq for $name<$($t),*> {
             #[inline]
             fn eq(&self, other: &Self) -> bool {
-                self.is_equal(&*other)
+                self.is_equal(other.as_ref())
             }
         }
 

objc2-foundation/src/string.rs

@@ -140,7 +140,7 @@ impl NSString {
         //
         // https://developer.apple.com/documentation/foundation/nsstring/1411189-utf8string?language=objc
         let bytes: *const c_char = unsafe { msg_send![self, UTF8String] };
-        let bytes = bytes as *const u8;
+        let bytes: *const u8 = bytes.cast();
         let len = self.len();
 
         // SAFETY:
@@ -199,7 +199,7 @@ impl NSString {
 }
 
 pub(crate) fn from_str(cls: &Class, string: &str) -> *mut Object {
-    let bytes = string.as_ptr() as *const c_void;
+    let bytes: *const c_void = string.as_ptr().cast();
     unsafe {
         let obj: *mut Object = msg_send![cls, alloc];
         msg_send![
@@ -276,6 +276,7 @@ impl fmt::Display for NSString {
 mod tests {
     use super::*;
     use alloc::format;
+    use core::ptr;
 
     #[cfg(feature = "gnustep-1-7")]
     #[test]
@@ -365,11 +366,10 @@ mod tests {
     fn test_copy_nsstring_is_same() {
         let string1 = NSString::from_str("Hello, world!");
         let string2 = string1.copy();
-
-        let s1: *const NSString = &*string1;
-        let s2: *const NSString = &*string2;
-
-        assert_eq!(s1, s2, "Cloned NSString didn't have the same address");
+        assert!(
+            ptr::eq(&*string1, &*string2),
+            "Cloned NSString didn't have the same address"
+        );
     }
 
     #[test]

objc2-foundation/src/value.rs

@@ -52,7 +52,7 @@ impl<T: 'static + Copy + Encode> NSValue<T> {
     /// The user must ensure that the inner value is properly initialized.
     pub unsafe fn get_unchecked(&self) -> T {
         let mut value = MaybeUninit::<T>::uninit();
-        let ptr = value.as_mut_ptr() as *mut c_void;
+        let ptr: *mut c_void = value.as_mut_ptr().cast();
         let _: () = unsafe { msg_send![self, getValue: ptr] };
         unsafe { value.assume_init() }
     }
@@ -64,7 +64,8 @@ impl<T: 'static + Copy + Encode> NSValue<T> {
 
     pub fn new(value: T) -> Id<Self, Shared> {
         let cls = Self::class();
-        let bytes = &value as *const T as *const c_void;
+        let bytes: *const T = &value;
+        let bytes: *const c_void = bytes.cast();
         let encoding = CString::new(T::ENCODING.to_string()).unwrap();
         unsafe {
             let obj: *mut Self = msg_send![cls, alloc];

objc2/examples/talk_to_me.rs

@@ -14,10 +14,11 @@ fn main() {
     const UTF8_ENCODING: NSUInteger = 4;
 
     let string: *const Object = unsafe { msg_send![class!(NSString), alloc] };
+    let text_ptr: *const c_void = text.as_ptr().cast();
     let string = unsafe {
         msg_send![
             string,
-            initWithBytes: text.as_ptr() as *const c_void,
+            initWithBytes: text_ptr,
             length: text.len(),
             encoding: UTF8_ENCODING,
         ]

objc2/src/cache.rs

@@ -27,9 +27,9 @@ impl CachedSel {
         // `Relaxed` should be fine since `sel_registerName` is thread-safe.
         let ptr = self.ptr.load(Ordering::Relaxed);
         if ptr.is_null() {
-            let ptr = unsafe { ffi::sel_registerName(name.as_ptr() as *const _) };
-            self.ptr.store(ptr as *mut _, Ordering::Relaxed);
-            unsafe { Sel::from_ptr(ptr as *const _) }
+            let ptr: *const c_void = unsafe { ffi::sel_registerName(name.as_ptr().cast()).cast() };
+            self.ptr.store(ptr as *mut c_void, Ordering::Relaxed);
+            unsafe { Sel::from_ptr(ptr) }
         } else {
             unsafe { Sel::from_ptr(ptr) }
         }
@@ -57,12 +57,12 @@ impl CachedClass {
     pub unsafe fn get(&self, name: &str) -> Option<&'static Class> {
         // `Relaxed` should be fine since `objc_getClass` is thread-safe.
         let ptr = self.ptr.load(Ordering::Relaxed);
-        if ptr.is_null() {
-            let cls = unsafe { ffi::objc_getClass(name.as_ptr() as *const _) } as *const Class;
-            self.ptr.store(cls as *mut _, Ordering::Relaxed);
-            unsafe { cls.as_ref() }
+        if let Some(cls) = unsafe { ptr.as_ref() } {
+            Some(cls)
         } else {
-            Some(unsafe { &*ptr })
+            let ptr: *const Class = unsafe { ffi::objc_getClass(name.as_ptr().cast()) }.cast();
+            self.ptr.store(ptr as *mut Class, Ordering::Relaxed);
+            unsafe { ptr.as_ref() }
         }
     }
 }