From 8c16687e5029c90ce51183653b652b59094f5b04 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 1 Nov 2024 03:24:06 +0100 Subject: [PATCH] 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 6a02cd27c..45c2c0a9a 100644 --- a/crates/objc2/src/exception.rs +++ b/crates/objc2/src/exception.rs @@ -239,7 +239,7 @@ pub fn throw(exception: Retained) -> ! { } #[cfg(feature = "exception")] -unsafe fn try_no_ret(closure: F) -> Result<(), Option>> { +fn try_no_ret(closure: F) -> Result<(), Option>> { let f = { extern "C-unwind" fn try_objc_execute_closure(closure: &mut Option) where @@ -291,8 +291,9 @@ unsafe fn try_no_ret(closure: F) -> Result<(), Option(closure: F) -> Result<(), Option( +pub fn catch( closure: impl FnOnce() -> R + UnwindSafe, ) -> Result>> { let mut value = None; @@ -322,7 +329,7 @@ pub unsafe fn catch( 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 = msg_send_id![&*obj, copy]; - }) - }; + let result = catch(|| { + let _: Retained = unsafe { msg_send_id![&*obj, copy] }; + }); let err = result.unwrap_err().unwrap(); assert_eq!( @@ -397,7 +398,7 @@ mod tests { let obj: Retained = 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 ")); @@ -418,6 +419,6 @@ mod tests { #[test] #[should_panic = "test"] 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::new(); - let _obj: *mut NSObject = msg_send![&arr, objectAtIndex: 0usize]; - }) - }; + let res = catch(|| { + let arr: Retained> = NSArray::new(); + let _obj: *mut NSObject = unsafe { msg_send![&arr, objectAtIndex: 0usize] }; + }); let exc = res.unwrap_err().unwrap(); let name = "NSRangeException";