From f3aedb4bab9033f6190cd7f13e6d2a94885a6dc4 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Tue, 4 Jun 2024 22:14:44 +0200 Subject: [PATCH 1/2] Fuzz hashing collections with interior mutability --- crates/test-fuzz/Cargo.toml | 23 ++- crates/test-fuzz/README.md | 2 - .../corpus/collection_interior_mut/.gitkeep | 0 .../fuzz_targets/collection_interior_mut.rs | 158 ++++++++++++++++++ 4 files changed, 180 insertions(+), 3 deletions(-) create mode 100644 crates/test-fuzz/corpus/collection_interior_mut/.gitkeep create mode 100644 crates/test-fuzz/fuzz_targets/collection_interior_mut.rs 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..a39cc3cf9 --- /dev/null +++ b/crates/test-fuzz/fuzz_targets/collection_interior_mut.rs @@ -0,0 +1,158 @@ +//! 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. +#![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; + // Intentionally `Immutable` to see what breaks if we allow mutation. + type Mutability = mutability::Immutable; + 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); + }); +} From 7e6d69115300a7e726845990defaada9c80391fb Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Tue, 4 Jun 2024 21:55:55 +0200 Subject: [PATCH 2/2] Remove HasStableHash requirement from NSSet and NSDictionary This was added in an abundance of caution in #505 to fix #337, 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 UB). --- .../src/topics/about_generated/CHANGELOG.md | 5 ++ .../fuzz_targets/collection_interior_mut.rs | 8 ++-- crates/test-ui/ui/nsset_from_nsobject.rs | 6 --- crates/test-ui/ui/nsset_from_nsobject.stderr | 22 --------- .../objc2-foundation/src/dictionary.rs | 29 ++++++------ framework-crates/objc2-foundation/src/set.rs | 46 +++++++------------ .../objc2-foundation/src/tests/set.rs | 7 ++- 7 files changed, 46 insertions(+), 77 deletions(-) delete mode 100644 crates/test-ui/ui/nsset_from_nsobject.rs delete mode 100644 crates/test-ui/ui/nsset_from_nsobject.stderr 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/fuzz_targets/collection_interior_mut.rs b/crates/test-fuzz/fuzz_targets/collection_interior_mut.rs index a39cc3cf9..8eb197295 100644 --- a/crates/test-fuzz/fuzz_targets/collection_interior_mut.rs +++ b/crates/test-fuzz/fuzz_targets/collection_interior_mut.rs @@ -1,7 +1,10 @@ //! 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. +//! 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; @@ -48,8 +51,7 @@ declare_class!( unsafe impl ClassType for Key { type Super = NSObject; - // Intentionally `Immutable` to see what breaks if we allow mutation. - type Mutability = mutability::Immutable; + type Mutability = mutability::InteriorMutable; const NAME: &'static str = "Key"; } 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()]); +}