diff --git a/crates/objc2/src/topics/about_generated/CHANGELOG.md b/crates/objc2/src/topics/about_generated/CHANGELOG.md index 7d9734029..a13957450 100644 --- a/crates/objc2/src/topics/about_generated/CHANGELOG.md +++ b/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`. diff --git a/crates/test-fuzz/Cargo.toml b/crates/test-fuzz/Cargo.toml index 181d5283f..6399ede27 100644 --- a/crates/test-fuzz/Cargo.toml +++ b/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 diff --git a/crates/test-fuzz/README.md b/crates/test-fuzz/README.md index 7e39c4601..ae2b804e8 100644 --- a/crates/test-fuzz/README.md +++ b/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 ``` diff --git a/crates/test-fuzz/corpus/collection_interior_mut/.gitkeep b/crates/test-fuzz/corpus/collection_interior_mut/.gitkeep new file mode 100644 index 000000000..e69de29bb diff --git a/crates/test-fuzz/fuzz_targets/collection_interior_mut.rs b/crates/test-fuzz/fuzz_targets/collection_interior_mut.rs new file mode 100644 index 000000000..8eb197295 --- /dev/null +++ b/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: +//! +#![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, + equal_to_mask: Cell, +} + +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.retain() + } + } +); + +impl Key { + fn new(index: KeyIndex) -> Retained { + 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) { + let keys: Vec<_> = (0..=KeyIndex::MAX).map(Key::new).collect(); + let key = |idx: KeyIndex| -> &Key { &keys[idx as usize] }; + + let mut set: Id> = NSMutableSet::new(); + let mut dict: Id> = 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| run(ops)); + +#[cfg(feature = "afl")] +fn main() { + afl::fuzz!(|ops: Vec| { + run(ops); + }); +} diff --git a/crates/test-ui/ui/nsset_from_nsobject.rs b/crates/test-ui/ui/nsset_from_nsobject.rs deleted file mode 100644 index a05ba9fb0..000000000 --- a/crates/test-ui/ui/nsset_from_nsobject.rs +++ /dev/null @@ -1,6 +0,0 @@ -//! Test that `NSSet` can't be created from types with an unknown stable hash. -use objc2_foundation::{NSObject, NSSet}; - -fn main() { - let _ = NSSet::from_vec(vec![NSObject::new()]); -} diff --git a/crates/test-ui/ui/nsset_from_nsobject.stderr b/crates/test-ui/ui/nsset_from_nsobject.stderr deleted file mode 100644 index 927a6c0ed..000000000 --- a/crates/test-ui/ui/nsset_from_nsobject.stderr +++ /dev/null @@ -1,22 +0,0 @@ -error[E0277]: the trait bound `objc2::mutability::Root: objc2::mutability::MutabilityHashIsStable` is not satisfied - --> ui/nsset_from_nsobject.rs - | - | let _ = NSSet::from_vec(vec![NSObject::new()]); - | --------------- ^^^^^^^^^^^^^^^^^^^^^ the trait `objc2::mutability::MutabilityHashIsStable` is not implemented for `objc2::mutability::Root`, which is required by `NSObject: objc2::mutability::HasStableHash` - | | - | required by a bound introduced by this call - | - = help: the following other types implement trait `objc2::mutability::MutabilityHashIsStable`: - objc2::mutability::Immutable - objc2::mutability::ImmutableWithMutableSubclass - objc2::mutability::Mutable - objc2::mutability::MutableWithImmutableSuperclass - = note: required for `NSObject` to implement `objc2::mutability::HasStableHash` -note: required by a bound in `set::>::from_vec` - --> $WORKSPACE/framework-crates/objc2-foundation/src/set.rs - | - | pub fn from_vec(mut vec: Vec>) -> Retained - | -------- required by a bound in this associated function - | where - | T: HasStableHash, - | ^^^^^^^^^^^^^ required by this bound in `set::>::from_vec` diff --git a/framework-crates/objc2-foundation/src/dictionary.rs b/framework-crates/objc2-foundation/src/dictionary.rs index 58a10dbe6..32b9f5b2b 100644 --- a/framework-crates/objc2-foundation/src/dictionary.rs +++ b/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 NSDictionary { +impl NSDictionary { + // 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(keys: &[&Q], mut objects: Vec>) -> Retained where Q: Message + NSCopying + CounterpartOrSelf, @@ -56,20 +60,15 @@ impl NSDictionary { // 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. + // unsafe { Self::initWithObjects_forKeys_count(Self::alloc(), objects, keys, count) } } @@ -103,7 +102,7 @@ impl NSDictionary { } #[cfg(feature = "NSObject")] -impl NSMutableDictionary { +impl NSMutableDictionary { pub fn from_vec(keys: &[&Q], mut objects: Vec>) -> Retained where Q: Message + NSCopying + CounterpartOrSelf, @@ -311,7 +310,7 @@ impl NSDictionary { } } -impl NSMutableDictionary { +impl NSMutableDictionary { /// Inserts a key-value pair into the dictionary. /// /// If the dictionary did not have this key present, None is returned. diff --git a/framework-crates/objc2-foundation/src/set.rs b/framework-crates/objc2-foundation/src/set.rs index e1da089c2..e1f2ccd83 100644 --- a/framework-crates/objc2-foundation/src/set.rs +++ b/framework-crates/objc2-foundation/src/set.rs @@ -4,7 +4,7 @@ use alloc::vec::Vec; use core::fmt; use core::hash::Hash; -use objc2::mutability::{HasStableHash, IsIdCloneable, IsRetainable}; +use objc2::mutability::{IsIdCloneable, IsRetainable}; use objc2::rc::{Retained, RetainedFromIterator}; use objc2::{extern_methods, ClassType, Message}; @@ -56,10 +56,7 @@ impl NSSet { /// let strs = ["one", "two", "three"].map(NSString::from_str).to_vec(); /// let set = NSSet::from_vec(strs); /// ``` - pub fn from_vec(mut vec: Vec>) -> Retained - where - T: HasStableHash, - { + pub fn from_vec(mut vec: Vec>) -> Retained { let len = vec.len(); let ptr = util::retained_ptr_cast(vec.as_mut_ptr()); // SAFETY: Same as `NSArray::from_vec`. @@ -78,7 +75,7 @@ impl NSSet { /// ``` pub fn from_id_slice(slice: &[Retained]) -> Retained where - T: HasStableHash + IsIdCloneable, + T: IsIdCloneable, { let len = slice.len(); let ptr = util::retained_ptr_cast_const(slice.as_ptr()); @@ -88,7 +85,7 @@ impl NSSet { pub fn from_slice(slice: &[&T]) -> Retained where - T: HasStableHash + IsRetainable, + T: IsRetainable, { let len = slice.len(); let ptr = util::ref_ptr_cast_const(slice.as_ptr()); @@ -171,10 +168,7 @@ impl NSMutableSet { /// let strs = ["one", "two", "three"].map(NSString::from_str).to_vec(); /// let set = NSMutableSet::from_vec(strs); /// ``` - pub fn from_vec(mut vec: Vec>) -> Retained - where - T: HasStableHash, - { + pub fn from_vec(mut vec: Vec>) -> Retained { let len = vec.len(); let ptr = util::retained_ptr_cast(vec.as_mut_ptr()); // SAFETY: Same as `NSArray::from_vec`. @@ -193,7 +187,7 @@ impl NSMutableSet { /// ``` pub fn from_id_slice(slice: &[Retained]) -> Retained where - T: HasStableHash + IsIdCloneable, + T: IsIdCloneable, { let len = slice.len(); let ptr = util::retained_ptr_cast_const(slice.as_ptr()); @@ -203,7 +197,7 @@ impl NSMutableSet { pub fn from_slice(slice: &[&T]) -> Retained where - T: HasStableHash + IsRetainable, + T: IsRetainable, { let len = slice.len(); let ptr = util::ref_ptr_cast_const(slice.as_ptr()); @@ -376,7 +370,7 @@ impl NSMutableSet { #[doc(alias = "addObject:")] pub fn insert(&mut self, value: &T) -> bool where - T: HasStableHash + IsRetainable, + T: IsRetainable, { let contains_value = self.contains(value); // SAFETY: Because of the `T: IsRetainable` bound, it is safe for the @@ -400,10 +394,7 @@ impl NSMutableSet { /// assert_eq!(set.len(), 1); /// ``` #[doc(alias = "addObject:")] - pub fn insert_id(&mut self, value: Retained) -> bool - where - T: HasStableHash, - { + pub fn insert_id(&mut self, value: Retained) -> bool { let contains_value = self.contains(&value); // SAFETY: We've consumed ownership of the object. unsafe { self.addObject(&value) }; @@ -425,10 +416,7 @@ impl NSMutableSet { /// assert_eq!(set.remove(ns_string!("one")), false); /// ``` #[doc(alias = "removeObject:")] - pub fn remove(&mut self, value: &T) -> bool - where - T: HasStableHash, - { + pub fn remove(&mut self, value: &T) -> bool { let contains_value = self.contains(value); unsafe { self.removeObject(value) }; contains_value @@ -563,7 +551,7 @@ impl fmt::Debug for crate::Foundation::NSCountedSet } } -impl Extend> for NSMutableSet { +impl Extend> for NSMutableSet { fn extend>>(&mut self, iter: I) { iter.into_iter().for_each(move |item| { self.insert_id(item); @@ -571,7 +559,7 @@ impl Extend> for NSMutableSe } } -impl<'a, T: Message + Eq + Hash + HasStableHash + IsRetainable> Extend<&'a T> for NSMutableSet { +impl<'a, T: Message + Eq + Hash + IsRetainable> Extend<&'a T> for NSMutableSet { fn extend>(&mut self, iter: I) { iter.into_iter().for_each(move |item| { self.insert(item); @@ -579,23 +567,21 @@ impl<'a, T: Message + Eq + Hash + HasStableHash + IsRetainable> Extend<&'a T> fo } } -impl<'a, T: Message + Eq + Hash + HasStableHash + IsRetainable + 'a> RetainedFromIterator<&'a T> - for NSSet -{ +impl<'a, T: Message + Eq + Hash + IsRetainable + 'a> RetainedFromIterator<&'a T> for NSSet { fn id_from_iter>(iter: I) -> Retained { let vec = Vec::from_iter(iter); Self::from_slice(&vec) } } -impl RetainedFromIterator> for NSSet { +impl RetainedFromIterator> for NSSet { fn id_from_iter>>(iter: I) -> Retained { let vec = Vec::from_iter(iter); Self::from_vec(vec) } } -impl<'a, T: Message + Eq + Hash + HasStableHash + IsRetainable + 'a> RetainedFromIterator<&'a T> +impl<'a, T: Message + Eq + Hash + IsRetainable + 'a> RetainedFromIterator<&'a T> for NSMutableSet { fn id_from_iter>(iter: I) -> Retained { @@ -604,7 +590,7 @@ impl<'a, T: Message + Eq + Hash + HasStableHash + IsRetainable + 'a> RetainedFro } } -impl RetainedFromIterator> for NSMutableSet { +impl RetainedFromIterator> for NSMutableSet { fn id_from_iter>>(iter: I) -> Retained { let vec = Vec::from_iter(iter); Self::from_vec(vec) diff --git a/framework-crates/objc2-foundation/src/tests/set.rs b/framework-crates/objc2-foundation/src/tests/set.rs index 7ab05d9c3..2dc6ae473 100644 --- a/framework-crates/objc2-foundation/src/tests/set.rs +++ b/framework-crates/objc2-foundation/src/tests/set.rs @@ -4,7 +4,7 @@ use alloc::vec::Vec; use alloc::{format, vec}; -use crate::Foundation::{self, ns_string, NSNumber, NSSet, NSString}; +use crate::Foundation::{self, ns_string, NSNumber, NSObject, NSSet, NSString}; #[test] fn test_new() { @@ -210,3 +210,8 @@ fn invalid_generic() { let _ = set.get_any().unwrap().get(0).unwrap(); // something_interior_mutable.setAbc(...) } + +#[test] +fn new_from_nsobject() { + let _ = NSSet::from_vec(vec![NSObject::new()]); +}