Make exception catching safe

Author: madsmtm

crates/objc2/CHANGELOG.md

@@ -92,6 +92,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
   accept nullable function pointers.
 * **BREAKING**: Changed the signature of various `ffi` functions to use the
   proper `Bool` type instead of a typedef.
+* Made `exception::catch` safe.
 
 ### Deprecated
 * Merged and deprecated the following `ffi` types:
@@ -160,6 +161,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
   - `AnyClass::instance_variable`.
   - `AnyProtocol::get`.
   - `AnyProtocol::name`.
+* Clarified that `exception::catch` does not catch Rust panics.
 
 
 ## 0.5.2 - 2024-05-21

crates/objc2/src/exception.rs

@@ -239,7 +239,7 @@ pub fn throw(exception: Retained<Exception>) -> ! {
 }
 
 #[cfg(feature = "exception")]
-unsafe fn try_no_ret<F: FnOnce()>(closure: F) -> Result<(), Option<Retained<Exception>>> {
+fn try_no_ret<F: FnOnce()>(closure: F) -> Result<(), Option<Retained<Exception>>> {
     let f = {
         extern "C-unwind" fn try_objc_execute_closure<F>(closure: &mut Option<F>)
         where
@@ -291,8 +291,9 @@ unsafe fn try_no_ret<F: FnOnce()>(closure: F) -> Result<(), Option<Retained<Exce
 /// if one is thrown.
 ///
 /// This is the Objective-C equivalent of Rust's [`catch_unwind`].
-/// Accordingly, if your Rust code is compiled with `panic=abort` this cannot
-/// catch the exception.
+/// Accordingly, if your Rust code is compiled with `panic=abort`, or your
+/// Objective-C code with `-fno-objc-exceptions`, this cannot catch the
+/// exception.
 ///
 /// [`catch_unwind`]: std::panic::catch_unwind
 ///
@@ -309,20 +310,26 @@ unsafe fn try_no_ret<F: FnOnce()>(closure: F) -> Result<(), Option<Retained<Exce
 /// situations.
 ///
 ///
-/// # Safety
+/// # Panics
 ///
-/// The given closure must not panic (e.g. normal Rust unwinding into this
-/// causes undefined behaviour).
+/// This panics if the given closure panics.
+///
+/// That is, it completely ignores Rust unwinding and simply lets that pass
+/// through unchanged.
+///
+/// It may also not catch all Objective-C exceptions (such as exceptions
+/// thrown when handling the memory management of the exception). These are
+/// mostly theoretical, and should only happen in utmost exceptional cases.
 #[cfg(feature = "exception")]
-pub unsafe fn catch<R>(
+pub fn catch<R>(
     closure: impl FnOnce() -> R + UnwindSafe,
 ) -> Result<R, Option<Retained<Exception>>> {
     let mut value = None;
     let value_ref = &mut value;
     let closure = move || {
         *value_ref = Some(closure());
     };
-    let result = unsafe { try_no_ret(closure) };
+    let result = try_no_ret(closure);
     // If the try succeeded, value was set so it's safe to unwrap
     result.map(|()| value.unwrap_or_else(|| unreachable!()))
 }
@@ -341,12 +348,10 @@ mod tests {
     #[test]
     fn test_catch() {
         let mut s = "Hello".to_string();
-        let result = unsafe {
-            catch(move || {
-                s.push_str(", World!");
-                s
-            })
-        };
+        let result = catch(move || {
+            s.push_str(", World!");
+            s
+        });
         assert_eq!(result.unwrap(), "Hello, World!");
     }
 
@@ -357,14 +362,12 @@ mod tests {
     )]
     fn test_catch_null() {
         let s = "Hello".to_string();
-        let result = unsafe {
-            catch(move || {
-                if !s.is_empty() {
-                    ffi::objc_exception_throw(ptr::null_mut())
-                }
-                s.len()
-            })
-        };
+        let result = catch(move || {
+            if !s.is_empty() {
+                unsafe { ffi::objc_exception_throw(ptr::null_mut()) }
+            }
+            s.len()
+        });
         assert!(result.unwrap_err().is_none());
     }
 
@@ -376,11 +379,9 @@ mod tests {
     fn test_catch_unknown_selector() {
         let obj = AssertUnwindSafe(NSObject::new());
         let ptr = Retained::as_ptr(&obj);
-        let result = unsafe {
-            catch(|| {
-                let _: Retained<NSObject> = msg_send_id![&*obj, copy];
-            })
-        };
+        let result = catch(|| {
+            let _: Retained<NSObject> = unsafe { msg_send_id![&*obj, copy] };
+        });
         let err = result.unwrap_err().unwrap();
 
         assert_eq!(
@@ -397,7 +398,7 @@ mod tests {
         let obj: Retained<Exception> = unsafe { Retained::cast_unchecked(obj) };
         let ptr: *const Exception = &*obj;
 
-        let result = unsafe { catch(|| throw(obj)) };
+        let result = catch(|| throw(obj));
         let obj = result.unwrap_err().unwrap();
 
         assert_eq!(format!("{obj:?}"), format!("exception <NSObject: {ptr:p}>"));
@@ -422,6 +423,6 @@ mod tests {
         ignore = "panic won't start on 32-bit / w. fragile runtime, it'll just abort, since the runtime uses setjmp/longjump unwinding"
     )]
     fn does_not_catch_panic() {
-        let _ = unsafe { catch(|| panic!("test")) };
+        let _ = catch(|| panic!("test"));
     }
 }

crates/objc2/src/runtime/message_receiver.rs

@@ -424,10 +424,7 @@ pub unsafe trait MessageReceiver: private::Sealed + Sized {
         }
 
         // SAFETY: Upheld by caller
-        //
-        // The @catch is safe since message sending primitives are guaranteed
-        // to do Objective-C compatible unwinding.
-        unsafe { conditional_try!(|| msg_send_primitive::send(receiver, sel, args)) }
+        conditional_try!(|| unsafe { msg_send_primitive::send(receiver, sel, args) })
     }
 
     /// Sends a message to a specific superclass with the given selector and
@@ -469,10 +466,10 @@ pub unsafe trait MessageReceiver: private::Sealed + Sized {
             msg_send_check_class(superclass, sel, A::ENCODINGS, &R::ENCODING_RETURN);
         }
 
-        // SAFETY: Same as in `send_message`
-        unsafe {
-            conditional_try!(|| msg_send_primitive::send_super(receiver, superclass, sel, args))
-        }
+        // SAFETY: Upheld by caller
+        conditional_try!(|| unsafe {
+            msg_send_primitive::send_super(receiver, superclass, sel, args)
+        })
     }
 }
 

crates/tests/src/exception.rs

@@ -22,7 +22,7 @@ fn throw_catch_raise_catch() {
 
     let exc = autoreleasepool(|_| {
         let exc = NSException::into_exception(exc);
-        let res = unsafe { catch(|| throw(exc)) };
+        let res = catch(|| throw(exc));
         let exc = res.unwrap_err().unwrap();
         let exc = NSException::from_exception(exc).unwrap();
 
@@ -33,14 +33,13 @@ fn throw_catch_raise_catch() {
     assert_eq!(exc.retainCount(), 1);
 
     let exc = autoreleasepool(|_| {
-        let inner = || {
+        let res = catch(|| {
             autoreleasepool(|pool| {
                 let exc = unsafe { Retained::autorelease(exc, pool) };
                 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
@@ -92,14 +91,15 @@ fn raise_catch() {
 
     let exc = autoreleasepool(|pool| {
         let exc = unsafe { Retained::autorelease(exc, pool) };
-        let inner = || {
+        let res = catch(|| {
             if exc.name() == name {
                 exc.raise();
             } else {
                 42
             }
-        };
-        let res = unsafe { catch(inner) }.unwrap_err().unwrap();
+        })
+        .unwrap_err()
+        .unwrap();
         assert_eq!(exc.retainCount(), 2);
         res
     });
@@ -120,12 +120,10 @@ fn raise_catch() {
     ignore = "Panics inside `catch` when catch-all is enabled"
 )]
 fn catch_actual() {
-    let res = unsafe {
-        catch(|| {
-            let arr: Retained<NSArray<NSObject>> = NSArray::new();
-            let _obj: *mut NSObject = msg_send![&arr, objectAtIndex: 0usize];
-        })
-    };
+    let res = catch(|| {
+        let arr: Retained<NSArray<NSObject>> = NSArray::new();
+        let _obj: *mut NSObject = unsafe { msg_send![&arr, objectAtIndex: 0usize] };
+    });
     let exc = res.unwrap_err().unwrap();
 
     let name = "NSRangeException";