From 01088d6ec54f54aee413466e85153dcbe96d7cdb Mon Sep 17 00:00:00 2001
From: Mads Marquart <mads@marquart.dk>
Date: Thu, 14 Nov 2024 16:30:23 +0100
Subject: [PATCH 1/3] Fix Rust panics in `exception::catch`

---
 crates/objc2-exception-helper/CHANGELOG.md    |  2 ++
 crates/objc2-exception-helper/src/lib.rs      | 11 +++++--
 crates/objc2-exception-helper/src/try_catch.m | 20 +++++++++++++
 crates/objc2/src/exception.rs                 | 30 +++++++++++++++----
 4 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/crates/objc2-exception-helper/CHANGELOG.md b/crates/objc2-exception-helper/CHANGELOG.md
index 2202553b3..1aa6fd456 100644
--- a/crates/objc2-exception-helper/CHANGELOG.md
+++ b/crates/objc2-exception-helper/CHANGELOG.md
@@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Fixed
 * Fixed the symbol name to include the correct SemVer version of the crate.
+* Fixed the ABI of `try_catch` to be `extern "C-unwind"`.
+* Clarified that `try_catch` does not catch Rust panics.
 
 
 ## 0.1.0 - 2024-06-02
diff --git a/crates/objc2-exception-helper/src/lib.rs b/crates/objc2-exception-helper/src/lib.rs
index f0b8ffb07..80737e54d 100644
--- a/crates/objc2-exception-helper/src/lib.rs
+++ b/crates/objc2-exception-helper/src/lib.rs
@@ -20,8 +20,9 @@ use core::ffi::c_void;
 
 type TryCatchClosure = extern "C-unwind" fn(*mut c_void);
 
-// `try_catch` is deliberately `extern "C"`, we just prevented the unwind.
-extern "C" {
+// `try_catch` is `extern "C-unwind"`, since it does not use `@catch (...)`,
+// but instead let unhandled exceptions pass through.
+extern "C-unwind" {
     /// Call the given function inside an Objective-C `@try/@catch` block.
     ///
     /// Defined in `src/try_catch.m` and compiled in `build.rs`.
@@ -37,6 +38,12 @@ extern "C" {
     /// (using mechanisms like core::intrinsics::r#try) is not an option!
     ///
     /// [manual-asm]: https://gitlab.com/objrs/objrs/-/blob/b4f6598696b3fa622e6fddce7aff281770b0a8c2/src/exception.rs
+    ///
+    ///
+    /// # Panics
+    ///
+    /// This panics / continues unwinding if the unwind is not triggered by an
+    /// Objective-C exception (i.e. it was triggered by Rust/C++/...).
     #[link_name = "objc2_exception_helper_0_1_try_catch"]
     pub fn try_catch(f: TryCatchClosure, context: *mut c_void, error: *mut *mut c_void) -> u8;
 }
diff --git a/crates/objc2-exception-helper/src/try_catch.m b/crates/objc2-exception-helper/src/try_catch.m
index 400706946..448747b11 100644
--- a/crates/objc2-exception-helper/src/try_catch.m
+++ b/crates/objc2-exception-helper/src/try_catch.m
@@ -17,7 +17,27 @@ unsigned char objc2_exception_helper_0_1_try_catch(void (*f)(void *), void *cont
     } @catch (id 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!
+        //
+        // Code throwing an exception know that they don't hold sole access to
+        // that object any more, so it's certainly safe to retain.
         *error = objc_retain(exception);
+        // NOTE: The above `retain` can throw, so an extra landing pad will be
+        // inserted here for that case.
         return 1;
     }
+    // NOTE: @catch(...) exists, but it only works reliably in 64-bit. On
+    // 32-bit, an exception may continue past that frame (I think).
+    // https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Exceptions/Articles/Exceptions64Bit.html
+    //
+    // Given that there is no way for us to reliably catch all Rust/C++/...
+    // exceptions here, the best choice is to consistently let them continue
+    // unwinding up the stack frame.
+    //
+    // Besides, not re-throwing Rust exceptions is not currently supported,
+    // so we'd just get an `abort` if we _did_ try to handle it:
+    // https://github.com/rust-lang/rust/blob/80d0d927d5069b67cc08c0c65b48e7b6e0cdeeb5/library/std/src/panicking.rs#L51-L58
+    //
+    // In any case, this _is_ the behaviour we want, to just catch Objective-C
+    // exceptions, C++/Rust/... exceptions are better handled with
+    // `std::panic::catch_unwind`.
 }
diff --git a/crates/objc2/src/exception.rs b/crates/objc2/src/exception.rs
index 09fc7126a..007063982 100644
--- a/crates/objc2/src/exception.rs
+++ b/crates/objc2/src/exception.rs
@@ -261,6 +261,18 @@ unsafe fn try_no_ret<F: FnOnce()>(closure: F) -> Result<(), Option<Retained<Exce
     let context = context.cast();
 
     let mut exception = ptr::null_mut();
+    // SAFETY: The function pointer and context are valid.
+    //
+    // The exception catching itself is sound on the Rust side, because we
+    // correctly use `extern "C-unwind"`. Objective-C does not completely
+    // specify how foreign unwinds are handled, though they do have the
+    // `@catch (...)` construct intended for catching C++ exceptions, so it is
+    // likely that they intend to support Rust panics (and it works in
+    // practice).
+    //
+    // See also:
+    // https://github.com/rust-lang/rust/pull/128321
+    // https://github.com/rust-lang/reference/pull/1226
     let success = unsafe { objc2_exception_helper::try_catch(f, context, &mut exception) };
 
     if success == 0 {
@@ -269,12 +281,8 @@ unsafe fn try_no_ret<F: FnOnce()>(closure: F) -> Result<(), Option<Retained<Exce
         // 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 `Retained` to it here.
+        // Since we do a retain in `objc2_exception_helper/src/try_catch.m`,
+        // the object has +1 retain count.
         Err(unsafe { Retained::from_raw(exception.cast()) })
     }
 }
@@ -406,4 +414,14 @@ mod tests {
         let result = catch_unwind(|| throw(obj));
         let _ = result.unwrap_err();
     }
+
+    #[test]
+    #[should_panic = "test"]
+    #[cfg_attr(
+        all(target_os = "macos", target_arch = "x86", panic = "unwind"),
+        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")) };
+    }
 }

From 76567af1343895de3ca134756382cbf50913d02a Mon Sep 17 00:00:00 2001
From: Mads Marquart <mads@marquart.dk>
Date: Fri, 1 Nov 2024 03:24:06 +0100
Subject: [PATCH 2/3] Make exception catching safe

---
 crates/objc2/CHANGELOG.md                    |  2 +
 crates/objc2/src/exception.rs                | 59 ++++++++++----------
 crates/objc2/src/runtime/message_receiver.rs | 13 ++---
 crates/tests/src/exception.rs                | 24 ++++----
 4 files changed, 48 insertions(+), 50 deletions(-)

diff --git a/crates/objc2/CHANGELOG.md b/crates/objc2/CHANGELOG.md
index 61724a1ef..5b2952c90 100644
--- a/crates/objc2/CHANGELOG.md
+++ b/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
diff --git a/crates/objc2/src/exception.rs b/crates/objc2/src/exception.rs
index 007063982..1527afea8 100644
--- a/crates/objc2/src/exception.rs
+++ b/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,12 +310,18 @@ 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;
@@ -322,7 +329,7 @@ pub unsafe fn catch<R>(
     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"));
     }
 }
diff --git a/crates/objc2/src/runtime/message_receiver.rs b/crates/objc2/src/runtime/message_receiver.rs
index 1acc6fcfc..4534b5f0c 100644
--- a/crates/objc2/src/runtime/message_receiver.rs
+++ b/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)
+        })
     }
 }
 
diff --git a/crates/tests/src/exception.rs b/crates/tests/src/exception.rs
index bc7c03920..ddcee0018 100644
--- a/crates/tests/src/exception.rs
+++ b/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";

From 74dda7a06d38eca2a088f87783d53334491880eb Mon Sep 17 00:00:00 2001
From: Mads Marquart <mads@marquart.dk>
Date: Fri, 15 Nov 2024 09:02:35 +0100
Subject: [PATCH 3/3] Update documentation on catch-all to say that it doesn't
 affect UB

---
 crates/objc2/src/exception.rs | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/crates/objc2/src/exception.rs b/crates/objc2/src/exception.rs
index 1527afea8..74081e7ff 100644
--- a/crates/objc2/src/exception.rs
+++ b/crates/objc2/src/exception.rs
@@ -1,10 +1,14 @@
 //! # `@throw` and `@try/@catch` exceptions.
 //!
-//! By default, if the [`msg_send!`] macro causes an exception to be thrown,
-//! this will unwind into Rust, resulting in undefined behavior. However, this
-//! crate has an `"catch-all"` feature which, when enabled, wraps each
-//! [`msg_send!`] in a `@catch` and panics if an exception is caught,
-//! preventing Objective-C from unwinding into Rust.
+//! By default, if a message send (such as those generated with the
+//! [`msg_send!`] and [`extern_methods!`] macros) causes an exception to be
+//! thrown, `objc2` will simply let it unwind into Rust.
+//!
+//! While not UB, it will likely end up aborting the process, since Rust
+//! cannot catch foreign exceptions like Objective-C's. However, `objc2` has
+//! the `"catch-all"` Cargo feature, which, when enabled, wraps each message
+//! send in a `@catch` and instead panics if an exception is caught, which
+//! might lead to slightly better error messages.
 //!
 //! Most of the functionality in this module is only available when the
 //! `"exception"` feature is enabled.