Remove HasStableHash requirement from NSSet and NSDictionary

crates/objc2/src/topics/about_generated/CHANGELOG.md

@@ -22,6 +22,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ### Changed
 * `objc2-foundation`: Allow using `MainThreadBound` without the `NSThread`
   feature flag.
+* `objc2-foundation`: Removed `HasStableHash` requirement on `NSDictionary` and
+  `NSSet` creation methods. This was added in an abundance of caution, but
+  prevents real-world usage of these types, and isn't actually needed for
+  soundness (the documentation mentions the collection being "corrupt" if the
+  hash is changed, but that's not the same as it being unsound).
 
 ### Deprecated
 * `objc2-foundation`: Moved `MainThreadMarker` to `objc2`.

crates/test-fuzz/Cargo.toml

@@ -28,7 +28,14 @@ gnustep-2-1 = ["gnustep-2-0", "objc2-foundation/gnustep-2-1"]
 afl = ["dep:afl"]
 
 # The features required for fuzzing all targets (used by CI)
-fuzz-all = ["objc2-foundation/NSString"]
+fuzz-all = [
+    "objc2-foundation/NSDictionary",
+    "objc2-foundation/NSEnumerator",
+    "objc2-foundation/NSObject",
+    "objc2-foundation/NSSet",
+    "objc2-foundation/NSString",
+    "objc2-foundation/NSZone",
+]
 
 [[bin]]
 name = "class"
@@ -59,5 +66,19 @@ doc = false
 bench = false
 required-features = ["objc2-foundation/NSString"]
 
+[[bin]]
+name = "collection_interior_mut"
+path = "fuzz_targets/collection_interior_mut.rs"
+test = false
+doc = false
+bench = false
+required-features = [
+    "objc2-foundation/NSDictionary",
+    "objc2-foundation/NSEnumerator",
+    "objc2-foundation/NSObject",
+    "objc2-foundation/NSSet",
+    "objc2-foundation/NSZone",
+]
+
 [package.metadata.release]
 release = false

crates/test-fuzz/README.md

@@ -13,8 +13,6 @@ Fuzz with AFL++ by doing:
 (This is probably not the optimal settings, AFL has a lot of configuration options).
 
 ```sh
-mkdir crates/test-fuzz/corpus/$fuzz_target
-mkdir -p crates/test-fuzz/afl
 cargo afl build --bin $fuzz_target --features=afl,fuzz-all --release
 cargo afl fuzz -i crates/test-fuzz/corpus/$fuzz_target -o crates/test-fuzz/afl -- target/release/$fuzz_target
 ```

crates/test-fuzz/fuzz_targets/collection_interior_mut.rs

@@ -0,0 +1,160 @@
+//! Fuzz hashing collection operations with interior mutability.
+//!
+//! This is explicitly not done with any form of oracle, since while this is
+//! not language-level undefined behaviour, the behaviour is not specified,
+//! and will "corrupt the collection", and may behave differently on different
+//! versions of Foundation:
+//! <https://developer.apple.com/library/archive/documentation/General/Conceptual/CocoaEncyclopedia/ObjectMutability/ObjectMutability.html#//apple_ref/doc/uid/TP40010810-CH5-SW69>
+#![cfg_attr(not(feature = "afl"), no_main)]
+use std::cell::Cell;
+use std::hint::black_box;
+
+use arbitrary::Arbitrary;
+use objc2::rc::{autoreleasepool, Id, Retained};
+use objc2::runtime::{AnyObject, ProtocolObject};
+use objc2::{declare_class, msg_send_id, mutability, ClassType, DeclaredClass};
+use objc2_foundation::{
+    NSCopying, NSMutableDictionary, NSMutableSet, NSObject, NSObjectProtocol, NSUInteger, NSZone,
+};
+
+/// Index into the global "keys" array.
+type KeyIndex = u8;
+
+/// The operations that the fuzzer can do on a set and the keys within.
+#[derive(Arbitrary, Debug)]
+enum Operation {
+    /// count
+    Count,
+    /// member: / objectForKey:
+    Get(KeyIndex),
+    /// objectEnumerator / keyEnumerator
+    Enumerate,
+    /// addObject: / setObject:forKey:
+    Add(KeyIndex),
+    /// removeObject: / removeObjectForKey:
+    Remove(KeyIndex),
+
+    /// Set the hash value of a key.
+    SetHash(KeyIndex, NSUInteger),
+    /// Set which other key masks this key is equal to.
+    SetEqualToMask(KeyIndex, u8),
+}
+
+struct KeyIvars {
+    index: KeyIndex,
+    hash: Cell<usize>,
+    equal_to_mask: Cell<u8>,
+}
+
+declare_class!(
+    struct Key;
+
+    unsafe impl ClassType for Key {
+        type Super = NSObject;
+        type Mutability = mutability::InteriorMutable;
+        const NAME: &'static str = "Key";
+    }
+
+    impl DeclaredClass for Key {
+        type Ivars = KeyIvars;
+    }
+
+    unsafe impl NSObjectProtocol for Key {
+        #[method(isEqual:)]
+        fn is_equal(&self, other: &AnyObject) -> bool {
+            assert_eq!(other.class(), Self::class());
+            let other: *const AnyObject = other;
+            let other: *const Self = other.cast();
+            // SAFETY: Just checked that the object is of this class
+            let other: &Self = unsafe { &*other };
+
+            (other.ivars().index & self.ivars().equal_to_mask.get()) != 0
+        }
+
+        #[method(hash)]
+        fn hash_(&self) -> NSUInteger {
+            self.ivars().hash.get()
+        }
+    }
+
+    unsafe impl NSCopying for Key {
+        #[method_id(copyWithZone:)]
+        fn copy_with_zone(&self, _zone: *mut NSZone) -> Retained<Self> {
+            self.retain()
+        }
+    }
+);
+
+impl Key {
+    fn new(index: KeyIndex) -> Retained<Self> {
+        let key = Key::alloc().set_ivars(KeyIvars {
+            index,
+            hash: Cell::new(0),
+            equal_to_mask: Cell::new(0),
+        });
+        unsafe { msg_send_id![super(key), init] }
+    }
+
+    fn validate(&self) {
+        black_box(self.ivars().index);
+        black_box(self.ivars().hash.get());
+    }
+}
+
+fn run(ops: Vec<Operation>) {
+    let keys: Vec<_> = (0..=KeyIndex::MAX).map(Key::new).collect();
+    let key = |idx: KeyIndex| -> &Key { &keys[idx as usize] };
+
+    let mut set: Id<NSMutableSet<Key>> = NSMutableSet::new();
+    let mut dict: Id<NSMutableDictionary<Key, NSObject>> = NSMutableDictionary::new();
+
+    for op in ops {
+        autoreleasepool(|_| match op {
+            Operation::Count => {
+                set.count();
+                dict.count();
+            }
+            Operation::Get(idx) => {
+                unsafe { set.member(key(idx)) };
+                unsafe { dict.objectForKey(key(idx)) };
+            }
+            Operation::Enumerate => {
+                for key in unsafe { set.objectEnumerator() } {
+                    key.validate();
+                }
+                for key in &set {
+                    key.validate();
+                }
+                for key in unsafe { dict.keyEnumerator() } {
+                    key.validate();
+                }
+            }
+            Operation::Add(idx) => {
+                unsafe { set.addObject(key(idx)) };
+                unsafe {
+                    dict.setObject_forKey(&NSObject::new(), ProtocolObject::from_ref(key(idx)))
+                };
+            }
+            Operation::Remove(idx) => {
+                unsafe { set.removeObject(key(idx)) };
+                dict.removeObjectForKey(key(idx));
+            }
+            Operation::SetHash(idx, hash) => {
+                key(idx).ivars().hash.set(hash);
+            }
+            Operation::SetEqualToMask(idx, equal_to_mask) => {
+                key(idx).ivars().equal_to_mask.set(equal_to_mask);
+            }
+        });
+    }
+}
+
+#[cfg(not(feature = "afl"))]
+libfuzzer_sys::fuzz_target!(|ops: Vec<Operation>| run(ops));
+
+#[cfg(feature = "afl")]
+fn main() {
+    afl::fuzz!(|ops: Vec<Operation>| {
+        run(ops);
+    });
+}

framework-crates/objc2-foundation/src/dictionary.rs

@@ -10,7 +10,7 @@ use core::ptr::{self, NonNull};
 
 #[cfg(feature = "NSObject")]
 use objc2::mutability::IsRetainable;
-use objc2::mutability::{CounterpartOrSelf, HasStableHash, IsIdCloneable, IsMutable};
+use objc2::mutability::{CounterpartOrSelf, IsIdCloneable, IsMutable};
 use objc2::rc::Retained;
 #[cfg(feature = "NSObject")]
 use objc2::runtime::ProtocolObject;
@@ -38,7 +38,11 @@ where
 }
 
 #[cfg(feature = "NSObject")]
-impl<K: Message + Eq + Hash + HasStableHash, V: Message> NSDictionary<K, V> {
+impl<K: Message + Eq + Hash, V: Message> NSDictionary<K, V> {
+    // The dictionary copies its keys, which is why we require `NSCopying` and
+    // use `CounterpartOrSelf` on all input data - we want to ensure that the
+    // type-system knows that it's not actually `NSMutableString` that is
+    // being stored, but instead `NSString`.
     pub fn from_vec<Q>(keys: &[&Q], mut objects: Vec<Retained<V>>) -> Retained<Self>
     where
         Q: Message + NSCopying + CounterpartOrSelf<Immutable = K>,
@@ -56,20 +60,15 @@ impl<K: Message + Eq + Hash + HasStableHash, V: Message> NSDictionary<K, V> {
         // SAFETY:
         // - The objects are valid, similar reasoning as `NSArray::from_vec`.
         //
-        // - The key must not be mutated, as that may cause the hash value to
-        //   change, which is unsound as stated in:
-        //   https://developer.apple.com/library/archive/documentation/General/Conceptual/CocoaEncyclopedia/ObjectMutability/ObjectMutability.html#//apple_ref/doc/uid/TP40010810-CH5-SW69
+        // - The length is lower than or equal to the length of the two arrays.
         //
-        //   The dictionary always copies its keys, which is why we require
-        //   `NSCopying` and use `CounterpartOrSelf` on all input data - we
-        //   want to ensure that it is very clear that it's not actually
-        //   `NSMutableString` that is being stored, but `NSString`.
+        // - While recommended against in the below link, the key _can_ be
+        //   mutated, it'll "just" corrupt the collection's invariants (but
+        //   won't cause undefined behaviour).
         //
-        //   But that is not by itself enough to verify that the key does not
-        //   still contain interior mutable objects (e.g. if the copy was only
-        //   a shallow copy), which is why we also require `HasStableHash`.
+        //   This is tested via. fuzzing in `collection_interior_mut.rs`.
         //
-        // - The length is lower than or equal to the length of the two arrays.
+        //   <https://developer.apple.com/library/archive/documentation/General/Conceptual/CocoaEncyclopedia/ObjectMutability/ObjectMutability.html#//apple_ref/doc/uid/TP40010810-CH5-SW69>
         unsafe { Self::initWithObjects_forKeys_count(Self::alloc(), objects, keys, count) }
     }
 
@@ -103,7 +102,7 @@ impl<K: Message + Eq + Hash + HasStableHash, V: Message> NSDictionary<K, V> {
 }
 
 #[cfg(feature = "NSObject")]
-impl<K: Message + Eq + Hash + HasStableHash, V: Message> NSMutableDictionary<K, V> {
+impl<K: Message + Eq + Hash, V: Message> NSMutableDictionary<K, V> {
     pub fn from_vec<Q>(keys: &[&Q], mut objects: Vec<Retained<V>>) -> Retained<Self>
     where
         Q: Message + NSCopying + CounterpartOrSelf<Immutable = K>,
@@ -311,7 +310,7 @@ impl<K: Message, V: Message> NSDictionary<K, V> {
     }
 }
 
-impl<K: Message + Eq + Hash + HasStableHash, V: Message> NSMutableDictionary<K, V> {
+impl<K: Message + Eq + Hash, V: Message> NSMutableDictionary<K, V> {
     /// Inserts a key-value pair into the dictionary.
     ///
     /// If the dictionary did not have this key present, None is returned.