From eb13e02f03b6c7ca2f98ccfdb49331d7ce269c0c Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Tue, 4 Jun 2024 21:55:55 +0200 Subject: [PATCH] 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 873d5f01f..072e23932 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()]); +}