Skip to content

Commit

Permalink
Merge pull request #626 from madsmtm/no-stable-hash
Browse files Browse the repository at this point in the history
Remove `HasStableHash` requirement from `NSSet` and `NSDictionary`
  • Loading branch information
madsmtm authored Jun 4, 2024
2 parents 0a723ab + 7e6d691 commit a820fb6
Show file tree
Hide file tree
Showing 10 changed files with 223 additions and 77 deletions.
5 changes: 5 additions & 0 deletions crates/objc2/src/topics/about_generated/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
23 changes: 22 additions & 1 deletion crates/test-fuzz/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
2 changes: 0 additions & 2 deletions crates/test-fuzz/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
```
Empty file.
160 changes: 160 additions & 0 deletions crates/test-fuzz/fuzz_targets/collection_interior_mut.rs
Original file line number Diff line number Diff line change
@@ -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);
});
}
6 changes: 0 additions & 6 deletions crates/test-ui/ui/nsset_from_nsobject.rs

This file was deleted.

22 changes: 0 additions & 22 deletions crates/test-ui/ui/nsset_from_nsobject.stderr

This file was deleted.

29 changes: 14 additions & 15 deletions framework-crates/objc2-foundation/src/dictionary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>,
Expand All @@ -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) }
}

Expand Down Expand Up @@ -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>,
Expand Down Expand Up @@ -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.
Expand Down
Loading

0 comments on commit a820fb6

Please sign in to comment.