From 0241c6bc0f2bd73296668a4802b41d2c2348081f Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 19 Jan 2022 22:06:11 +0100 Subject: [PATCH 1/9] Verify that owned Ids are unique using associated objects --- objc-sys/src/constants.rs | 38 ++++++++++++++++- objc2/src/rc/id.rs | 87 ++++++++++++++++++++++++++++++++++++--- objc2/src/rc/ownership.rs | 59 +++++++++++++++++++++++++- objc2/src/runtime.rs | 9 ++++ 4 files changed, 184 insertions(+), 9 deletions(-) diff --git a/objc-sys/src/constants.rs b/objc-sys/src/constants.rs index 362fc7097..36495621f 100644 --- a/objc-sys/src/constants.rs +++ b/objc-sys/src/constants.rs @@ -19,12 +19,32 @@ pub const nil: id = 0 as *mut _; /// A quick alias for a [`null_mut`][`core::ptr::null_mut`] class. pub const Nil: *mut objc_class = 0 as *mut _; +/// Policies related to associative references. +/// +/// These are options to [`objc_setAssociatedObject`]. +/// +/// [`objc_setAssociatedObject`]: crate::objc_setAssociatedObject pub type objc_AssociationPolicy = usize; +/// Specifies a weak reference to the associated object. +/// +/// This performs straight assignment, without message sends. pub const OBJC_ASSOCIATION_ASSIGN: objc_AssociationPolicy = 0; +/// Specifies a strong reference to the associated object. +/// +/// The association is not made atomically. pub const OBJC_ASSOCIATION_RETAIN_NONATOMIC: objc_AssociationPolicy = 1; +/// Specifies that the associated object is copied. +/// +/// The association is not made atomically. pub const OBJC_ASSOCIATION_COPY_NONATOMIC: objc_AssociationPolicy = 3; -pub const OBJC_ASSOCIATION_RETAIN: objc_AssociationPolicy = 769; -pub const OBJC_ASSOCIATION_COPY: objc_AssociationPolicy = 771; +/// Specifies a strong reference to the associated object. +/// +/// The association is made atomically. +pub const OBJC_ASSOCIATION_RETAIN: objc_AssociationPolicy = 0o1401; +/// Specifies that the associated object is copied. +/// +/// The association is made atomically. +pub const OBJC_ASSOCIATION_COPY: objc_AssociationPolicy = 0o1403; #[cfg(apple)] pub const OBJC_SYNC_SUCCESS: c_int = 0; @@ -36,3 +56,17 @@ pub const OBJC_SYNC_TIMED_OUT: c_int = -2; /// Only relevant before macOS 10.13 #[cfg(apple)] pub const OBJC_SYNC_NOT_INITIALIZED: c_int = -3; + +#[cfg(test)] +mod tests { + use super::*; + #[test] + fn test_association_policy() { + assert_eq!(OBJC_ASSOCIATION_RETAIN, 769); + assert_eq!(OBJC_ASSOCIATION_COPY, 771); + + // What the GNUStep headers define + assert_eq!(OBJC_ASSOCIATION_RETAIN, 0x301); + assert_eq!(OBJC_ASSOCIATION_COPY, 0x303); + } +} diff --git a/objc2/src/rc/id.rs b/objc2/src/rc/id.rs index fa71e305f..8e766e339 100644 --- a/objc2/src/rc/id.rs +++ b/objc2/src/rc/id.rs @@ -176,6 +176,10 @@ impl Id { // Note: We don't take a reference as a parameter since it would be too // easy to accidentally create two aliasing mutable references. pub unsafe fn new(ptr: *mut T) -> Option> { + #[cfg(debug_assertions)] + unsafe { + O::__ensure_unique_if_owned(ptr as *mut _) + }; // Should optimize down to nothing. // SAFETY: Upheld by the caller NonNull::new(ptr).map(|ptr| unsafe { Id::new_nonnull(ptr) }) @@ -216,6 +220,16 @@ impl Id { // SAFETY: Option> has the same size as *mut T unsafe { mem::transmute::>, *mut T>(ManuallyDrop::new(obj)) } } + + // TODO: Pub? + #[inline] + fn forget(self) -> *mut T { + #[cfg(debug_assertions)] + unsafe { + O::__relinquish_ownership(self.as_ptr() as *mut _) + }; + ManuallyDrop::new(self).as_ptr() + } } impl Id { @@ -410,11 +424,11 @@ impl Id { // retained objects it is hard to imagine a case where the inner type // has a method with the same name. - let ptr = ManuallyDrop::new(self).ptr.as_ptr(); + let ptr = self.forget(); // SAFETY: The `ptr` is guaranteed to be valid and have at least one // retain count. - // And because of the ManuallyDrop, we don't call the Drop - // implementation, so the object won't also be released there. + // And because of the `forget`, we don't call the Drop implementation, + // so the object won't also be released there. let res: *mut T = unsafe { ffi::objc_autorelease(ptr.cast()) }.cast(); debug_assert_eq!(res, ptr, "objc_autorelease did not return the same pointer"); res @@ -467,7 +481,8 @@ impl Id { #[inline] pub unsafe fn from_shared(obj: Id) -> Self { // Note: We can't debug_assert retainCount because of autoreleases - let ptr = ManuallyDrop::new(obj).ptr; + let ptr = obj.ptr; + obj.forget(); // SAFETY: The pointer is valid // Ownership rules are upheld by the caller unsafe { >::new_nonnull(ptr) } @@ -496,7 +511,8 @@ impl From> for Id { /// Downgrade from an owned to a shared [`Id`], allowing it to be cloned. #[inline] fn from(obj: Id) -> Self { - let ptr = ManuallyDrop::new(obj).ptr; + let ptr = obj.ptr; + obj.forget(); // SAFETY: The pointer is valid, and ownership is simply decreased unsafe { >::new_nonnull(ptr) } } @@ -534,6 +550,11 @@ impl Drop for Id { #[doc(alias = "release")] #[inline] fn drop(&mut self) { + #[cfg(debug_assertions)] + unsafe { + O::__relinquish_ownership(self.ptr.as_ptr() as *mut _) + }; + // We could technically run the destructor for `T` when `O = Owned`, // and when `O = Shared` with (retainCount == 1), but that would be // confusing and inconsistent since we cannot guarantee that it's run. @@ -626,6 +647,15 @@ mod tests { assert_eq!(retain_count, expected); } + fn new() -> Id { + let cls = class!(NSObject); + unsafe { + let obj: *mut Object = msg_send![cls, alloc]; + let obj: *mut Object = msg_send![obj, init]; + Id::new(NonNull::new_unchecked(obj)) + } + } + #[test] fn test_drop() { let mut expected = ThreadTestData::current(); @@ -704,4 +734,51 @@ mod tests { expected.retain += 1; expected.assert_current(); } + + #[test] + fn test_unique_owned() { + let obj = new(); + + // To/from shared + let obj = obj.into(); + let obj: Id = unsafe { Id::from_shared(obj) }; + + // To/from autoreleased + let obj: Id = autoreleasepool(|pool| { + let obj = obj.autorelease(pool); + unsafe { Id::retain_null(obj).unwrap() } + }); + + // Forget and bring back + let ptr = obj.forget(); + let _obj: Id = unsafe { Id::new_null(ptr).unwrap() }; + } + + #[test] + #[cfg_attr( + debug_assertions, + should_panic = "Another `Id` has already been created from that object" + )] + fn test_double_owned() { + let obj = new(); + // Double-retain: This is unsound! + let _obj2: Id = unsafe { Id::retain_null(obj.as_ptr()).unwrap() }; + } + + #[test] + #[cfg_attr( + debug_assertions, + should_panic = "Tried to give up ownership of `Id` that wasn't owned" + )] + fn test_double_forgotten() { + let obj = new(); + let ptr = obj.forget(); + + let _obj: Id = Id { + ptr: NonNull::new(ptr).unwrap(), + item: PhantomData, + own: PhantomData, + }; + // Dropping fails + } } diff --git a/objc2/src/rc/ownership.rs b/objc2/src/rc/ownership.rs index 1ea2cddd2..2af6c43a0 100644 --- a/objc2/src/rc/ownership.rs +++ b/objc2/src/rc/ownership.rs @@ -1,3 +1,7 @@ +use core::ffi::c_void; + +use crate::ffi; + /// A type used to mark that a struct owns the object(s) it contains, /// so it has the sole references to them. #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)] @@ -20,7 +24,58 @@ mod private { /// /// This trait is sealed and not meant to be implemented outside of the this /// crate. -pub trait Ownership: private::Sealed + 'static {} +pub trait Ownership: private::Sealed + 'static { + #[doc(hidden)] + #[inline(always)] + unsafe fn __ensure_unique_if_owned(_: *mut ffi::objc_object) {} + #[doc(hidden)] + #[inline(always)] + unsafe fn __relinquish_ownership(_: *mut ffi::objc_object) {} +} + +/// The value of this doesn't matter, it's only the address that we use. +static OBJC2_ID_OWNED_UNIQUE_KEY: i32 = 0; +#[inline(always)] +fn key() -> *const c_void { + &OBJC2_ID_OWNED_UNIQUE_KEY as *const i32 as *const _ +} + +impl Ownership for Owned { + #[track_caller] + unsafe fn __ensure_unique_if_owned(obj: *mut ffi::objc_object) { + std::println!("\n__ensure_unique_if_owned: {:?} / {:?}", obj, key()); + let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; + std::println!("associated: {:?}", associated); + if associated.is_null() { + // Set the associated object to something (it can be anything, so + // we just set it to the current object). + // + // We use `assign` because we don't want to retain or copy the + // object, since we just use it as a marker. + unsafe { ffi::objc_setAssociatedObject(obj, key(), obj, ffi::OBJC_ASSOCIATION_ASSIGN) }; + } else { + panic!("Another `Id` has already been created from that object!"); + } + let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; + std::println!("associated: {:?}", associated); + } + + #[track_caller] + unsafe fn __relinquish_ownership(obj: *mut ffi::objc_object) { + std::println!("\n__relinquish_ownership: {:?} / {:?}", obj, key()); + let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; + std::println!("associated: {:?}", associated); + if associated.is_null() { + panic!("Tried to give up ownership of `Id` that wasn't owned!"); + } else { + // Clear the associated object + unsafe { + ffi::objc_setAssociatedObject(obj, key(), ffi::nil, ffi::OBJC_ASSOCIATION_ASSIGN) + }; + } + let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; + std::println!("associated: {:?}", associated); + } +} -impl Ownership for Owned {} impl Ownership for Shared {} diff --git a/objc2/src/runtime.rs b/objc2/src/runtime.rs index 0a8dd4530..67607f28a 100644 --- a/objc2/src/runtime.rs +++ b/objc2/src/runtime.rs @@ -662,10 +662,19 @@ impl Object { // SAFETY: Invariants upheld by caller unsafe { *self.ivar_mut::(name) = value }; } +} +/// Associated objects. +/// +/// TODO: Make a public API for these. +/// +/// See [Apple's documentation](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ObjectiveC/Chapters/ocAssociativeReferences.html). +impl Object { // objc_setAssociatedObject // objc_getAssociatedObject // objc_removeAssociatedObjects + + // https://nshipster.com/associated-objects/ } unsafe impl RefEncode for Object { From d2a1b61c3414a3c1a80376cbf0c714d295eaca60 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 19 Jan 2022 22:06:20 +0100 Subject: [PATCH 2/9] Verify ownership when creating and releasing shared Ids as well --- objc2/src/rc/id.rs | 10 ++++++++++ objc2/src/rc/ownership.rs | 27 ++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/objc2/src/rc/id.rs b/objc2/src/rc/id.rs index 8e766e339..589b717f1 100644 --- a/objc2/src/rc/id.rs +++ b/objc2/src/rc/id.rs @@ -781,4 +781,14 @@ mod tests { }; // Dropping fails } + + #[test] + #[cfg_attr( + debug_assertions, + should_panic = "An `Id` exists while trying to create `Id`!" + )] + fn test_create_shared_when_owned_exists() { + let obj = new(); + let _obj: Id = unsafe { Id::retain(obj.ptr) }; + } } diff --git a/objc2/src/rc/ownership.rs b/objc2/src/rc/ownership.rs index 2af6c43a0..ec0b72e64 100644 --- a/objc2/src/rc/ownership.rs +++ b/objc2/src/rc/ownership.rs @@ -26,11 +26,9 @@ mod private { /// crate. pub trait Ownership: private::Sealed + 'static { #[doc(hidden)] - #[inline(always)] - unsafe fn __ensure_unique_if_owned(_: *mut ffi::objc_object) {} + unsafe fn __ensure_unique_if_owned(_: *mut ffi::objc_object); #[doc(hidden)] - #[inline(always)] - unsafe fn __relinquish_ownership(_: *mut ffi::objc_object) {} + unsafe fn __relinquish_ownership(_: *mut ffi::objc_object); } /// The value of this doesn't matter, it's only the address that we use. @@ -78,4 +76,23 @@ impl Ownership for Owned { } } -impl Ownership for Shared {} +impl Ownership for Shared { + #[track_caller] + unsafe fn __ensure_unique_if_owned(obj: *mut ffi::objc_object) { + std::println!("\n__ensure_unique_if_owned: {:?} / {:?}", obj, key()); + let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; + std::println!("associated: {:?}", associated); + if !associated.is_null() { + panic!("An `Id` exists while trying to create `Id`!"); + } + } + #[track_caller] + unsafe fn __relinquish_ownership(obj: *mut ffi::objc_object) { + std::println!("\n__relinquish_ownership: {:?} / {:?}", obj, key()); + let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; + std::println!("associated: {:?}", associated); + if !associated.is_null() { + panic!("Tried to give up ownership of `Id`!"); + } + } +} From 9cb51a9981ce4225bee286bb2a1af6d64e982fe8 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 24 Jun 2022 21:37:27 +0200 Subject: [PATCH 3/9] Fix for newer objc2 --- objc2/src/rc/id.rs | 58 +++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/objc2/src/rc/id.rs b/objc2/src/rc/id.rs index 589b717f1..47d7e8743 100644 --- a/objc2/src/rc/id.rs +++ b/objc2/src/rc/id.rs @@ -175,18 +175,15 @@ impl Id { #[inline] // Note: We don't take a reference as a parameter since it would be too // easy to accidentally create two aliasing mutable references. - pub unsafe fn new(ptr: *mut T) -> Option> { - #[cfg(debug_assertions)] - unsafe { - O::__ensure_unique_if_owned(ptr as *mut _) - }; + pub unsafe fn new(ptr: *mut T) -> Option { + unsafe { O::__ensure_unique_if_owned(ptr.cast()) }; // Should optimize down to nothing. // SAFETY: Upheld by the caller - NonNull::new(ptr).map(|ptr| unsafe { Id::new_nonnull(ptr) }) + NonNull::new(ptr).map(|ptr| unsafe { Self::new_nonnull(ptr) }) } #[inline] - unsafe fn new_nonnull(ptr: NonNull) -> Id { + unsafe fn new_nonnull(ptr: NonNull) -> Self { Self { ptr, item: PhantomData, @@ -203,12 +200,13 @@ impl Id { /// /// This is an associated method, and must be called as `Id::as_ptr(obj)`. #[inline] - pub fn as_ptr(this: &Id) -> *const T { + pub fn as_ptr(this: &Self) -> *const T { this.ptr.as_ptr() } #[inline] pub(crate) fn consume_as_ptr(this: ManuallyDrop) -> *mut T { + unsafe { O::__relinquish_ownership(this.ptr.as_ptr().cast()) }; this.ptr.as_ptr() } @@ -218,17 +216,17 @@ impl Id { // So we just hack it with transmute! // SAFETY: Option> has the same size as *mut T - unsafe { mem::transmute::>, *mut T>(ManuallyDrop::new(obj)) } + let obj = ManuallyDrop::new(obj); + let ptr = unsafe { mem::transmute::>, *mut T>(obj) }; + unsafe { O::__relinquish_ownership(ptr.cast()) }; + ptr } // TODO: Pub? #[inline] - fn forget(self) -> *mut T { - #[cfg(debug_assertions)] - unsafe { - O::__relinquish_ownership(self.as_ptr() as *mut _) - }; - ManuallyDrop::new(self).as_ptr() + fn forget(this: Self) -> *mut T { + unsafe { O::__relinquish_ownership(this.ptr.as_ptr().cast()) }; + ManuallyDrop::new(this).ptr.as_ptr() } } @@ -424,7 +422,7 @@ impl Id { // retained objects it is hard to imagine a case where the inner type // has a method with the same name. - let ptr = self.forget(); + let ptr = Self::forget(self); // SAFETY: The `ptr` is guaranteed to be valid and have at least one // retain count. // And because of the `forget`, we don't call the Drop implementation, @@ -482,7 +480,7 @@ impl Id { pub unsafe fn from_shared(obj: Id) -> Self { // Note: We can't debug_assert retainCount because of autoreleases let ptr = obj.ptr; - obj.forget(); + Id::forget(obj); // SAFETY: The pointer is valid // Ownership rules are upheld by the caller unsafe { >::new_nonnull(ptr) } @@ -512,7 +510,7 @@ impl From> for Id { #[inline] fn from(obj: Id) -> Self { let ptr = obj.ptr; - obj.forget(); + Id::forget(obj); // SAFETY: The pointer is valid, and ownership is simply decreased unsafe { >::new_nonnull(ptr) } } @@ -550,10 +548,7 @@ impl Drop for Id { #[doc(alias = "release")] #[inline] fn drop(&mut self) { - #[cfg(debug_assertions)] - unsafe { - O::__relinquish_ownership(self.ptr.as_ptr() as *mut _) - }; + unsafe { O::__relinquish_ownership(self.ptr.as_ptr().cast()) }; // We could technically run the destructor for `T` when `O = Owned`, // and when `O = Shared` with (retainCount == 1), but that would be @@ -652,7 +647,7 @@ mod tests { unsafe { let obj: *mut Object = msg_send![cls, alloc]; let obj: *mut Object = msg_send![obj, init]; - Id::new(NonNull::new_unchecked(obj)) + Id::new(obj).unwrap() } } @@ -746,12 +741,12 @@ mod tests { // To/from autoreleased let obj: Id = autoreleasepool(|pool| { let obj = obj.autorelease(pool); - unsafe { Id::retain_null(obj).unwrap() } + unsafe { Id::retain(obj).unwrap() } }); // Forget and bring back - let ptr = obj.forget(); - let _obj: Id = unsafe { Id::new_null(ptr).unwrap() }; + let ptr = Id::forget(obj); + let _obj: Id = unsafe { Id::new(ptr).unwrap() }; } #[test] @@ -760,9 +755,9 @@ mod tests { should_panic = "Another `Id` has already been created from that object" )] fn test_double_owned() { - let obj = new(); + let mut obj = new(); // Double-retain: This is unsound! - let _obj2: Id = unsafe { Id::retain_null(obj.as_ptr()).unwrap() }; + let _obj2: Id = unsafe { Id::retain(Id::as_mut_ptr(&mut obj)).unwrap() }; } #[test] @@ -772,12 +767,13 @@ mod tests { )] fn test_double_forgotten() { let obj = new(); - let ptr = obj.forget(); + let ptr = Id::forget(obj); let _obj: Id = Id { ptr: NonNull::new(ptr).unwrap(), item: PhantomData, own: PhantomData, + notunwindsafe: PhantomData, }; // Dropping fails } @@ -788,7 +784,7 @@ mod tests { should_panic = "An `Id` exists while trying to create `Id`!" )] fn test_create_shared_when_owned_exists() { - let obj = new(); - let _obj: Id = unsafe { Id::retain(obj.ptr) }; + let mut obj = new(); + let _obj: Id = unsafe { Id::retain(Id::as_mut_ptr(&mut obj)).unwrap() }; } } From f01a75651d8d430e3268adfad03b0b7012c2d93f Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 24 Jun 2022 21:42:35 +0200 Subject: [PATCH 4/9] Add feature flag "unstable-verify-ownership" --- objc2/Cargo.toml | 4 ++++ objc2/src/rc/id.rs | 18 ++++++------------ objc2/src/rc/ownership.rs | 18 +++++++++++++++--- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/objc2/Cargo.toml b/objc2/Cargo.toml index bb03c89e2..4567b5c58 100644 --- a/objc2/Cargo.toml +++ b/objc2/Cargo.toml @@ -58,6 +58,10 @@ unstable-autoreleasesafe = [] # Uses the nightly c_unwind feature to make throwing safe unstable-c-unwind = ["objc-sys/unstable-c-unwind"] +# Uses associated types on every object to check whether Id's ownership rules +# are being upheld. +unstable-verify-ownership = [] + # Runtime selection. See `objc-sys` for details. apple = ["objc-sys/apple"] gnustep-1-7 = ["objc-sys/gnustep-1-7"] diff --git a/objc2/src/rc/id.rs b/objc2/src/rc/id.rs index 47d7e8743..3cafbfcfe 100644 --- a/objc2/src/rc/id.rs +++ b/objc2/src/rc/id.rs @@ -750,10 +750,8 @@ mod tests { } #[test] - #[cfg_attr( - debug_assertions, - should_panic = "Another `Id` has already been created from that object" - )] + #[cfg(feature = "unstable-verify-ownership")] + #[should_panic = "Another `Id` has already been created from that object"] fn test_double_owned() { let mut obj = new(); // Double-retain: This is unsound! @@ -761,10 +759,8 @@ mod tests { } #[test] - #[cfg_attr( - debug_assertions, - should_panic = "Tried to give up ownership of `Id` that wasn't owned" - )] + #[cfg(feature = "unstable-verify-ownership")] + #[should_panic = "Tried to give up ownership of `Id` that wasn't owned"] fn test_double_forgotten() { let obj = new(); let ptr = Id::forget(obj); @@ -779,10 +775,8 @@ mod tests { } #[test] - #[cfg_attr( - debug_assertions, - should_panic = "An `Id` exists while trying to create `Id`!" - )] + #[cfg(feature = "unstable-verify-ownership")] + #[should_panic = "An `Id` exists while trying to create `Id`!"] fn test_create_shared_when_owned_exists() { let mut obj = new(); let _obj: Id = unsafe { Id::retain(Id::as_mut_ptr(&mut obj)).unwrap() }; diff --git a/objc2/src/rc/ownership.rs b/objc2/src/rc/ownership.rs index ec0b72e64..a124e3e50 100644 --- a/objc2/src/rc/ownership.rs +++ b/objc2/src/rc/ownership.rs @@ -26,18 +26,23 @@ mod private { /// crate. pub trait Ownership: private::Sealed + 'static { #[doc(hidden)] - unsafe fn __ensure_unique_if_owned(_: *mut ffi::objc_object); + #[inline] + unsafe fn __ensure_unique_if_owned(_: *mut ffi::objc_object) {} #[doc(hidden)] - unsafe fn __relinquish_ownership(_: *mut ffi::objc_object); + #[inline] + unsafe fn __relinquish_ownership(_: *mut ffi::objc_object) {} } /// The value of this doesn't matter, it's only the address that we use. static OBJC2_ID_OWNED_UNIQUE_KEY: i32 = 0; -#[inline(always)] + +#[inline] +#[allow(unused)] fn key() -> *const c_void { &OBJC2_ID_OWNED_UNIQUE_KEY as *const i32 as *const _ } +#[cfg(feature = "unstable-verify-ownership")] impl Ownership for Owned { #[track_caller] unsafe fn __ensure_unique_if_owned(obj: *mut ffi::objc_object) { @@ -76,6 +81,10 @@ impl Ownership for Owned { } } +#[cfg(not(feature = "unstable-verify-ownership"))] +impl Ownership for Owned {} + +#[cfg(feature = "unstable-verify-ownership")] impl Ownership for Shared { #[track_caller] unsafe fn __ensure_unique_if_owned(obj: *mut ffi::objc_object) { @@ -96,3 +105,6 @@ impl Ownership for Shared { } } } + +#[cfg(not(feature = "unstable-verify-ownership"))] +impl Ownership for Shared {} From 46320616fa658e514bc6fc5364a41ab0a0631376 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Sat, 25 Jun 2022 00:48:07 +0200 Subject: [PATCH 5/9] Fix tests with verified ownership --- objc2/src/__macro_helpers.rs | 2 ++ objc2/src/rc/id.rs | 14 +++++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/objc2/src/__macro_helpers.rs b/objc2/src/__macro_helpers.rs index 2c3422caf..cfa477952 100644 --- a/objc2/src/__macro_helpers.rs +++ b/objc2/src/__macro_helpers.rs @@ -272,6 +272,8 @@ mod tests { // let copy: Id = unsafe { msg_send_id![&obj, copy].unwrap() }; // let mutable_copy: Id = unsafe { msg_send_id![&obj, mutableCopy].unwrap() }; + let obj: Id = obj.into(); + expected.assert_current(); let _self: Id = unsafe { msg_send_id![&obj, self].unwrap() }; expected.retain += 1; expected.assert_current(); diff --git a/objc2/src/rc/id.rs b/objc2/src/rc/id.rs index 3cafbfcfe..6e472c7e6 100644 --- a/objc2/src/rc/id.rs +++ b/objc2/src/rc/id.rs @@ -176,7 +176,6 @@ impl Id { // Note: We don't take a reference as a parameter since it would be too // easy to accidentally create two aliasing mutable references. pub unsafe fn new(ptr: *mut T) -> Option { - unsafe { O::__ensure_unique_if_owned(ptr.cast()) }; // Should optimize down to nothing. // SAFETY: Upheld by the caller NonNull::new(ptr).map(|ptr| unsafe { Self::new_nonnull(ptr) }) @@ -184,6 +183,7 @@ impl Id { #[inline] unsafe fn new_nonnull(ptr: NonNull) -> Self { + unsafe { O::__ensure_unique_if_owned(ptr.as_ptr().cast()) }; Self { ptr, item: PhantomData, @@ -228,6 +228,12 @@ impl Id { unsafe { O::__relinquish_ownership(this.ptr.as_ptr().cast()) }; ManuallyDrop::new(this).ptr.as_ptr() } + + #[inline] + fn forget_nonnull(this: Self) -> NonNull { + unsafe { O::__relinquish_ownership(this.ptr.as_ptr().cast()) }; + ManuallyDrop::new(this).ptr + } } impl Id { @@ -479,8 +485,7 @@ impl Id { #[inline] pub unsafe fn from_shared(obj: Id) -> Self { // Note: We can't debug_assert retainCount because of autoreleases - let ptr = obj.ptr; - Id::forget(obj); + let ptr = Id::forget_nonnull(obj); // SAFETY: The pointer is valid // Ownership rules are upheld by the caller unsafe { >::new_nonnull(ptr) } @@ -509,8 +514,7 @@ impl From> for Id { /// Downgrade from an owned to a shared [`Id`], allowing it to be cloned. #[inline] fn from(obj: Id) -> Self { - let ptr = obj.ptr; - Id::forget(obj); + let ptr = Id::forget_nonnull(obj); // SAFETY: The pointer is valid, and ownership is simply decreased unsafe { >::new_nonnull(ptr) } } From 5e8bcec73dbfb1f557baa7b1dc5649567dfc85d9 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Sat, 25 Jun 2022 00:48:14 +0200 Subject: [PATCH 6/9] Code cleanup --- objc2/src/rc/id.rs | 31 ++++++++++++------------------- objc2/src/rc/ownership.rs | 20 ++++++++++---------- 2 files changed, 22 insertions(+), 29 deletions(-) diff --git a/objc2/src/rc/id.rs b/objc2/src/rc/id.rs index 6e472c7e6..eafb7d862 100644 --- a/objc2/src/rc/id.rs +++ b/objc2/src/rc/id.rs @@ -646,15 +646,6 @@ mod tests { assert_eq!(retain_count, expected); } - fn new() -> Id { - let cls = class!(NSObject); - unsafe { - let obj: *mut Object = msg_send![cls, alloc]; - let obj: *mut Object = msg_send![obj, init]; - Id::new(obj).unwrap() - } - } - #[test] fn test_drop() { let mut expected = ThreadTestData::current(); @@ -736,40 +727,41 @@ mod tests { #[test] fn test_unique_owned() { - let obj = new(); + // Tests patterns that `unstable-verify-ownership` should allow + let obj = RcTestObject::new(); // To/from shared let obj = obj.into(); - let obj: Id = unsafe { Id::from_shared(obj) }; + let obj: Id<_, Owned> = unsafe { Id::from_shared(obj) }; // To/from autoreleased - let obj: Id = autoreleasepool(|pool| { + let obj: Id<_, Owned> = autoreleasepool(|pool| { let obj = obj.autorelease(pool); unsafe { Id::retain(obj).unwrap() } }); // Forget and bring back let ptr = Id::forget(obj); - let _obj: Id = unsafe { Id::new(ptr).unwrap() }; + let _obj: Id<_, Owned> = unsafe { Id::new(ptr).unwrap() }; } #[test] #[cfg(feature = "unstable-verify-ownership")] #[should_panic = "Another `Id` has already been created from that object"] fn test_double_owned() { - let mut obj = new(); + let mut obj = RcTestObject::new(); // Double-retain: This is unsound! - let _obj2: Id = unsafe { Id::retain(Id::as_mut_ptr(&mut obj)).unwrap() }; + let _obj2: Id<_, Owned> = unsafe { Id::retain(&mut *obj).unwrap() }; } #[test] #[cfg(feature = "unstable-verify-ownership")] #[should_panic = "Tried to give up ownership of `Id` that wasn't owned"] fn test_double_forgotten() { - let obj = new(); + let obj = RcTestObject::new(); let ptr = Id::forget(obj); - let _obj: Id = Id { + let _obj: Id<_, Owned> = Id { ptr: NonNull::new(ptr).unwrap(), item: PhantomData, own: PhantomData, @@ -782,7 +774,8 @@ mod tests { #[cfg(feature = "unstable-verify-ownership")] #[should_panic = "An `Id` exists while trying to create `Id`!"] fn test_create_shared_when_owned_exists() { - let mut obj = new(); - let _obj: Id = unsafe { Id::retain(Id::as_mut_ptr(&mut obj)).unwrap() }; + let obj = RcTestObject::new(); + let ptr: *const RcTestObject = &*obj; + let _obj: Id<_, Shared> = unsafe { Id::retain(ptr as *mut RcTestObject).unwrap() }; } } diff --git a/objc2/src/rc/ownership.rs b/objc2/src/rc/ownership.rs index a124e3e50..e2e39839c 100644 --- a/objc2/src/rc/ownership.rs +++ b/objc2/src/rc/ownership.rs @@ -46,9 +46,9 @@ fn key() -> *const c_void { impl Ownership for Owned { #[track_caller] unsafe fn __ensure_unique_if_owned(obj: *mut ffi::objc_object) { - std::println!("\n__ensure_unique_if_owned: {:?} / {:?}", obj, key()); + std::println!("\nOwned __ensure_unique_if_owned: {:?} / {:?}", obj, key()); let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; - std::println!("associated: {:?}", associated); + std::println!("Owned associated: {:?}", associated); if associated.is_null() { // Set the associated object to something (it can be anything, so // we just set it to the current object). @@ -60,14 +60,14 @@ impl Ownership for Owned { panic!("Another `Id` has already been created from that object!"); } let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; - std::println!("associated: {:?}", associated); + std::println!("Owned associated: {:?}", associated); } #[track_caller] unsafe fn __relinquish_ownership(obj: *mut ffi::objc_object) { - std::println!("\n__relinquish_ownership: {:?} / {:?}", obj, key()); + std::println!("\nOwned __relinquish_ownership: {:?} / {:?}", obj, key()); let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; - std::println!("associated: {:?}", associated); + std::println!("Owned associated: {:?}", associated); if associated.is_null() { panic!("Tried to give up ownership of `Id` that wasn't owned!"); } else { @@ -77,7 +77,7 @@ impl Ownership for Owned { }; } let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; - std::println!("associated: {:?}", associated); + std::println!("Owned associated: {:?}", associated); } } @@ -88,18 +88,18 @@ impl Ownership for Owned {} impl Ownership for Shared { #[track_caller] unsafe fn __ensure_unique_if_owned(obj: *mut ffi::objc_object) { - std::println!("\n__ensure_unique_if_owned: {:?} / {:?}", obj, key()); + std::println!("\nShared __ensure_unique_if_owned: {:?} / {:?}", obj, key()); let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; - std::println!("associated: {:?}", associated); + std::println!("Shared associated: {:?}", associated); if !associated.is_null() { panic!("An `Id` exists while trying to create `Id`!"); } } #[track_caller] unsafe fn __relinquish_ownership(obj: *mut ffi::objc_object) { - std::println!("\n__relinquish_ownership: {:?} / {:?}", obj, key()); + std::println!("\nShared __relinquish_ownership: {:?} / {:?}", obj, key()); let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; - std::println!("associated: {:?}", associated); + std::println!("Shared associated: {:?}", associated); if !associated.is_null() { panic!("Tried to give up ownership of `Id`!"); } From ccc578170e2d0c25a2f96e63eff939ff756fbef8 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Sat, 25 Jun 2022 00:55:05 +0200 Subject: [PATCH 7/9] Add NULL check in ownership verification --- objc2/src/rc/id.rs | 4 +++- objc2/src/rc/ownership.rs | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/objc2/src/rc/id.rs b/objc2/src/rc/id.rs index eafb7d862..f4cad0265 100644 --- a/objc2/src/rc/id.rs +++ b/objc2/src/rc/id.rs @@ -218,7 +218,9 @@ impl Id { // SAFETY: Option> has the same size as *mut T let obj = ManuallyDrop::new(obj); let ptr = unsafe { mem::transmute::>, *mut T>(obj) }; - unsafe { O::__relinquish_ownership(ptr.cast()) }; + if !ptr.is_null() { + unsafe { O::__relinquish_ownership(ptr.cast()) }; + } ptr } diff --git a/objc2/src/rc/ownership.rs b/objc2/src/rc/ownership.rs index e2e39839c..8030e6cb5 100644 --- a/objc2/src/rc/ownership.rs +++ b/objc2/src/rc/ownership.rs @@ -47,6 +47,10 @@ impl Ownership for Owned { #[track_caller] unsafe fn __ensure_unique_if_owned(obj: *mut ffi::objc_object) { std::println!("\nOwned __ensure_unique_if_owned: {:?} / {:?}", obj, key()); + if obj.is_null() { + panic!("Tried to take ownership of nil object!"); + } + let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; std::println!("Owned associated: {:?}", associated); if associated.is_null() { @@ -66,6 +70,10 @@ impl Ownership for Owned { #[track_caller] unsafe fn __relinquish_ownership(obj: *mut ffi::objc_object) { std::println!("\nOwned __relinquish_ownership: {:?} / {:?}", obj, key()); + if obj.is_null() { + panic!("Tried to release ownership of nil object!"); + } + let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; std::println!("Owned associated: {:?}", associated); if associated.is_null() { @@ -89,6 +97,10 @@ impl Ownership for Shared { #[track_caller] unsafe fn __ensure_unique_if_owned(obj: *mut ffi::objc_object) { std::println!("\nShared __ensure_unique_if_owned: {:?} / {:?}", obj, key()); + if obj.is_null() { + panic!("Tried to take shared ownership of nil object!"); + } + let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; std::println!("Shared associated: {:?}", associated); if !associated.is_null() { @@ -98,6 +110,10 @@ impl Ownership for Shared { #[track_caller] unsafe fn __relinquish_ownership(obj: *mut ffi::objc_object) { std::println!("\nShared __relinquish_ownership: {:?} / {:?}", obj, key()); + if obj.is_null() { + panic!("Tried to release shared ownership of nil object!"); + } + let associated = unsafe { ffi::objc_getAssociatedObject(obj, key()) }; std::println!("Shared associated: {:?}", associated); if !associated.is_null() { From 1264a0d5781c631620c60455fc58af94f3d424c0 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Sat, 25 Jun 2022 00:55:14 +0200 Subject: [PATCH 8/9] Run ownership verification in CI --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d4c9fcae8..c52809021 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -153,7 +153,7 @@ jobs: ARGS: --no-default-features --features std --features ${{ matrix.runtime || 'apple' }} ${{ matrix.args }} # Use --no-fail-fast, except with dinghy TESTARGS: ${{ matrix.dinghy && ' ' || '--no-fail-fast' }} ${{ matrix.test-args }} - FEATURES: ${{ matrix.features || 'malloc,block,exception,catch_all,verify_message' }} + FEATURES: ${{ matrix.features || 'malloc,block,exception,catch_all,verify_message,unstable-verify-ownership' }} UNSTABLE_FEATURES: ${{ matrix.unstable-features || 'unstable-autoreleasesafe,unstable-c-unwind' }} runs-on: ${{ matrix.os }} From 7b75f0ea6f336df1d042c4181cf67f34bca34c22 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Sat, 25 Jun 2022 00:59:04 +0200 Subject: [PATCH 9/9] Fix clippy --- objc2/src/rc/ownership.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/objc2/src/rc/ownership.rs b/objc2/src/rc/ownership.rs index 8030e6cb5..ca1d107db 100644 --- a/objc2/src/rc/ownership.rs +++ b/objc2/src/rc/ownership.rs @@ -39,7 +39,8 @@ static OBJC2_ID_OWNED_UNIQUE_KEY: i32 = 0; #[inline] #[allow(unused)] fn key() -> *const c_void { - &OBJC2_ID_OWNED_UNIQUE_KEY as *const i32 as *const _ + let ptr: *const i32 = &OBJC2_ID_OWNED_UNIQUE_KEY; + ptr.cast() } #[cfg(feature = "unstable-verify-ownership")]