Merge pull request #441 from madsmtm/improve-objc-sys

Fix memory leaks in exception handling

Author: madsmtm

crates/objc-sys/CHANGELOG.md

@@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## Unreleased - YYYY-MM-DD
 
+### Added
+* Improved documentation slightly.
+
 
 ## 0.3.0 - 2023-02-07
 

crates/objc-sys/build.rs

@@ -210,9 +210,7 @@ fn main() {
     // - ...
     //
     // TODO: -fobjc-weak ?
-    let mut cc_args = format!(
-        "-fobjc-arc -fobjc-arc-exceptions -fobjc-exceptions -fobjc-runtime={clang_runtime}"
-    );
+    let mut cc_args = format!("-fobjc-exceptions -fobjc-runtime={clang_runtime}");
 
     if let Runtime::ObjFW(_) = &runtime {
         // Add compability headers to make `#include <objc/objc.h>` work.
@@ -221,8 +219,6 @@ fn main() {
         cc_args.push_str(compat_headers.to_str().unwrap());
     }
 
-    println!("cargo:cc_args={cc_args}"); // DEP_OBJC_[version]_CC_ARGS
-
     if let Runtime::ObjFW(_) = &runtime {
         // Link to libobjfw-rt
         println!("cargo:rustc-link-lib=dylib=objfw-rt");
@@ -250,10 +246,18 @@ fn main() {
         let mut builder = cc::Build::new();
         builder.file("extern/exception.m");
 
+        // Compile with exceptions enabled and with the correct runtime, but
+        // _without ARC_!
         for flag in cc_args.split(' ') {
             builder.flag(flag);
         }
 
         builder.compile("librust_objc_sys_0_3_try_catch_exception.a");
     }
+
+    // Add this to the `CC` args _after_ we've omitted it when compiling
+    // `extern/exception.m`.
+    cc_args.push_str(" -fobjc-arc -fobjc-arc-exceptions");
+
+    println!("cargo:cc_args={cc_args}"); // DEP_OBJC_[version]_CC_ARGS
 }

crates/objc-sys/extern/exception.m

@@ -12,14 +12,11 @@
 unsigned char rust_objc_sys_0_3_try_catch_exception(void (*f)(void *), void *context, id *error) {
     @try {
         f(context);
-        if (error) {
-            *error = (id)0; // nil
-        }
         return 0;
     } @catch (id exception) {
-        if (error) {
-            *error = objc_retain(exception);
-        }
+        // The exception is retained while inside the `@catch` block, but is
+        // not guaranteed to be so outside of it; so hence we must do it here!
+        *error = objc_retain(exception);
         return 1;
     }
 }

crates/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(any(doc, 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(any(doc, 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);
+    }
+}

crates/objc-sys/src/image_info.rs

@@ -1,3 +1,6 @@
+// TODO: Move this to `objc2` once we can detect simulator targets without a
+// build script.
+
 /// Note: While `objc2` relies on this, you can freely break this, since it is
 /// only used behind experimental features (`unstable-static-*`).
 #[repr(C)]

crates/objc-sys/src/protocol.rs

@@ -30,12 +30,10 @@ extern_c! {
     #[cfg(any(doc, not(objfw)))]
     pub fn objc_registerProtocol(proto: *mut objc_protocol);
 
-    // TODO: Verify unwinding
     pub fn protocol_conformsToProtocol(
         proto: *const objc_protocol,
         other: *const objc_protocol,
     ) -> BOOL;
-    // TODO: Verify unwinding
     pub fn protocol_isEqual(proto: *const objc_protocol, other: *const objc_protocol) -> BOOL;
     pub fn protocol_getName(proto: *const objc_protocol) -> *const c_char;
 

crates/objc-sys/src/types.rs

@@ -105,14 +105,16 @@ pub type BOOL = inner::BOOL;
 //
 // Likewise for NSUInteger.
 //
+//
 // ## GNUStep / WinObjC
 //
 // Defined as intptr_t/uintptr_t, which is exactly the same as isize/usize.
 //
+//
 // ## ObjFW
 //
-// Doesn't define these, but e.g. `OFString -length` returns size_t, so our
-// definitions are should be correct on effectively all targets.
+// Doesn't define these, but e.g. -[OFString length] returns size_t, so our
+// definitions should be correct on effectively all targets.
 //
 // Things might change slightly in the future, see
 // <https://internals.rust-lang.org/t/pre-rfc-usize-is-not-size-t/15369>.
@@ -127,15 +129,23 @@ pub type BOOL = inner::BOOL;
 ///
 /// [docs]: https://developer.apple.com/documentation/objectivec/nsinteger?language=objc
 ///
+///
 /// # Examples
 ///
 /// ```
-/// #[repr(isize)] // NSInteger
+/// use core::mem::size_of;
+/// # use objc_sys::NSInteger;
+/// # #[cfg(not_available)]
+/// use objc2::ffi::NSInteger;
+///
+/// #[repr(isize)]
 /// pub enum NSComparisonResult {
 ///     NSOrderedAscending = -1,
 ///     NSOrderedSame = 0,
 ///     NSOrderedDescending = 1,
 /// }
+///
+/// assert_eq!(size_of::<NSComparisonResult>(), size_of::<NSInteger>());
 /// ```
 pub type NSInteger = isize;
 
@@ -149,36 +159,43 @@ pub type NSInteger = isize;
 ///
 /// [docs]: https://developer.apple.com/documentation/objectivec/nsuinteger?language=objc
 ///
+///
 /// # Examples
 ///
 /// ```
-/// use objc_sys::NSUInteger;
-/// // Or:
-/// // use objc2::ffi::NSUInteger;
-/// // use icrate::Foundation::NSUInteger;
+/// # use objc_sys::NSUInteger;
+/// # #[cfg(not_available)]
+/// use objc2::ffi::NSUInteger;
+///
 /// extern "C" {
 ///     fn some_external_function() -> NSUInteger;
 /// }
 /// ```
 ///
 /// ```
-/// #[repr(usize)] // NSUInteger
-/// enum NSRoundingMode {
-///     NSRoundPlain = 0,
-///     NSRoundDown = 1,
-///     NSRoundUp = 2,
-///     NSRoundBankers = 3,
-/// };
+/// use core::mem::size_of;
+/// # use objc_sys::NSUInteger;
+/// # #[cfg(not_available)]
+/// use objc2::ffi::NSUInteger;
+///
+/// #[repr(usize)]
+/// enum CLRegionState {
+///     Unknown = 0,
+///     Inside = 1,
+///     Outside = 2,
+/// }
+///
+/// assert_eq!(size_of::<CLRegionState>(), size_of::<NSUInteger>());
 /// ```
 pub type NSUInteger = usize;
 
-/// The maximum value for an NSInteger.
+/// The maximum value for a [`NSInteger`].
 pub const NSIntegerMax: NSInteger = NSInteger::MAX;
 
-/// The minimum value for an NSInteger.
+/// The minimum value for a [`NSInteger`].
 pub const NSIntegerMin: NSInteger = NSInteger::MIN;
 
-/// The maximum value for an NSUInteger.
+/// The maximum value for a [`NSUInteger`].
 pub const NSUIntegerMax: NSUInteger = NSUInteger::MAX;
 
 /// An immutable pointer to a selector.
@@ -189,7 +206,9 @@ pub type SEL = *const objc_selector;
 
 /// A mutable pointer to an object / instance.
 ///
-/// Type alias provided for convenience. See `objc2::runtime::Object` for a
-/// higher level binding, and `objc2::rc::Id` for an easier way of handling
-/// objects.
+/// Type alias provided for convenience. You'll likely want to use one of:
+/// - `icrate::Foundation::NS[...]` for when you know the class of the object
+///   you're dealing with.
+/// - `objc2::rc::Id` for a proper way of doing memory management.
+/// - `objc2::runtime::Object` for a bit safer representation of this.
 pub type id = *mut objc_object;

crates/objc2/CHANGELOG.md

@@ -68,6 +68,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Fixed
 * Fixed using autorelease pools on 32bit macOS and older macOS versions.
+* Fixed memory leaks in and improved performance of `exception::catch`.
 
 ### Removed
 * **BREAKING**: Removed `rc::SliceId`, since it is trivially implementable

crates/objc2/src/exception.rs

@@ -237,6 +237,9 @@ unsafe fn try_no_ret<F: FnOnce()>(closure: F) -> Result<(), Option<Id<Exception>
         // SAFETY:
         // The exception is always a valid object or NULL.
         //
+        // Since we do a retain inside `extern/exception.m`, the object has
+        // +1 retain count.
+        //
         // Code throwing an exception know that they don't hold sole access to
         // that object any more, so even if the type was originally mutable,
         // it is okay to create a new `Id` to it here.

crates/tests/src/exception.rs

@@ -22,29 +22,39 @@ fn throw_catch_raise_catch() {
     let reason = NSString::from_str("def");
 
     let exc = NSException::new(&name, Some(&reason), None).unwrap();
-    // TODO: Investigate why this is required on GNUStep!
-    let _exc2 = exc.clone();
 
-    assert_retain_count(&exc, 2);
-
-    // TODO: Investigate this!
-    let extra_retain = usize::from(cfg!(all(
-        feature = "apple",
-        target_os = "macos",
-        target_arch = "x86"
-    )));
+    assert_retain_count(&exc, 1);
 
     let exc = autoreleasepool(|_| {
         let exc = NSException::into_exception(exc);
         let res = unsafe { catch(|| throw(exc)) };
         let exc = res.unwrap_err().unwrap();
         let exc = NSException::from_exception(exc).unwrap();
 
-        assert_retain_count(&exc, 3 + extra_retain);
+        assert_retain_count(&exc, 1);
+        exc
+    });
+
+    assert_retain_count(&exc, 1);
+
+    let exc = autoreleasepool(|_| {
+        let inner = || {
+            autoreleasepool(|pool| {
+                let exc = Id::autorelease(exc, pool);
+                unsafe { exc.raise() }
+            })
+        };
+
+        let res = unsafe { catch(inner) };
+        let exc = NSException::from_exception(res.unwrap_err().unwrap()).unwrap();
+
+        // Undesired: The inner pool _should_ have been drained on unwind, but
+        // it isn't, see `rc::Pool::drain`.
+        assert_retain_count(&exc, 2);
         exc
     });
 
-    assert_retain_count(&exc, 2 + extra_retain);
+    assert_retain_count(&exc, 1);
 
     assert_eq!(exc.name(), name);
     assert_eq!(exc.reason().unwrap(), reason);
@@ -85,28 +95,26 @@ fn raise_catch() {
     let exc = NSException::new(&name, Some(&reason), None).unwrap();
     assert_retain_count(&exc, 1);
 
-    let res = autoreleasepool(|_| {
-        let res = unsafe {
-            catch(|| {
-                if exc.name() == name {
-                    exc.raise();
-                } else {
-                    4
-                }
-            })
+    let exc = autoreleasepool(|pool| {
+        let exc = Id::autorelease(exc, pool);
+        let inner = || {
+            if exc.name() == name {
+                unsafe { exc.raise() };
+            } else {
+                42
+            }
         };
-        assert_retain_count(&exc, 3);
+        let res = unsafe { catch(inner) }.unwrap_err().unwrap();
+        assert_retain_count(&exc, 2);
         res
     });
 
-    assert_retain_count(&exc, 2);
-
-    let obj = res.unwrap_err().unwrap();
+    assert_retain_count(&exc, 1);
 
-    assert_eq!(format!("{obj}"), "def");
+    assert_eq!(format!("{exc}"), "def");
     assert_eq!(
-        format!("{obj:?}"),
-        format!("exception <NSException: {:p}> 'abc' reason:def", &*obj)
+        format!("{exc:?}"),
+        format!("exception <NSException: {:p}> 'abc' reason:def", &*exc)
     );
 }