Change msg_send! such that callers can properly communicate mutability

madsmtm · madsmtm · commit 36912b071ee0 · 2022-06-06T18:48:56.000+02:00
This fixes a long-standing soundness issue with how message sending is done whilst mutating the receiver, see: SSheldon/rust-objc#112. We were effectively mutating behind either `&&mut T` or `&T`, where `T` is zero-sized and contains `UnsafeCell`, so while it is still uncertain exactly how much of an issue this actually is, the approach we use now is definitely sound! Also makes it clearer that `msg_send!` does not consume `Id`s, it only needs a reference to those.
diff --git a/objc2-foundation/examples/custom_class.rs b/objc2-foundation/examples/custom_class.rs
@@ -70,12 +70,12 @@ fn main() {
 
     obj.set_number(7);
     println!("Number: {}", unsafe {
-        let number: u32 = msg_send![obj, number];
+        let number: u32 = msg_send![&obj, number];
         number
     });
 
     unsafe {
-        let _: () = msg_send![obj, setNumber: 12u32];
+        let _: () = msg_send![&mut obj, setNumber: 12u32];
     }
     println!("Number: {}", obj.number());
 }
diff --git a/objc2-foundation/src/array.rs b/objc2-foundation/src/array.rs
@@ -274,13 +274,14 @@ impl<T: Message, O: Ownership> NSMutableArray<T, O> {
 
     #[doc(alias = "removeLastObject")]
     pub fn pop(&mut self) -> Option<Id<T, O>> {
-        self.last().map(|obj| {
-            let obj = unsafe { Id::retain(obj as *const T as *mut T).unwrap_unchecked() };
-            unsafe {
-                let _: () = msg_send![self, removeLastObject];
-            }
-            obj
-        })
+        self.last()
+            .map(|obj| unsafe { Id::retain(obj as *const T as *mut T).unwrap_unchecked() })
+            .map(|obj| {
+                unsafe {
+                    let _: () = msg_send![self, removeLastObject];
+                }
+                obj
+            })
     }
 
     #[doc(alias = "removeAllObjects")]
diff --git a/objc2-foundation/src/data.rs b/objc2-foundation/src/data.rs
@@ -156,7 +156,8 @@ impl NSMutableData {
 impl NSMutableData {
     #[doc(alias = "mutableBytes")]
     pub fn bytes_mut(&mut self) -> &mut [u8] {
-        let ptr: *mut c_void = unsafe { msg_send![self, mutableBytes] };
+        let this = &mut *self; // Reborrow
+        let ptr: *mut c_void = unsafe { msg_send![this, mutableBytes] };
         // The bytes pointer may be null for length zero
         if ptr.is_null() {
             &mut []
diff --git a/objc2-foundation/src/dictionary.rs b/objc2-foundation/src/dictionary.rs
@@ -129,7 +129,7 @@ impl<K: Message, V: Message> NSDictionary<K, V> {
 
     pub fn into_values_array(dict: Id<Self, Owned>) -> Id<NSArray<V, Owned>, Shared> {
         unsafe {
-            let vals = msg_send![dict, allValues];
+            let vals = msg_send![&dict, allValues];
             Id::retain_autoreleased(vals).unwrap()
         }
     }
diff --git a/objc2-foundation/src/enumerator.rs b/objc2-foundation/src/enumerator.rs
@@ -33,7 +33,7 @@ impl<'a, T: Message> Iterator for NSEnumerator<'a, T> {
     type Item = &'a T;
 
     fn next(&mut self) -> Option<&'a T> {
-        unsafe { msg_send![self.id, nextObject] }
+        unsafe { msg_send![&mut self.id, nextObject] }
     }
 }
 
diff --git a/objc2-foundation/src/value.rs b/objc2-foundation/src/value.rs
@@ -177,7 +177,7 @@ mod tests {
     fn test_value_nsrange() {
         let val = NSValue::new(NSRange::from(1..2));
         assert!(NSRange::ENCODING.equivalent_to_str(val.encoding().unwrap()));
-        let range: NSRange = unsafe { objc2::msg_send![val, rangeValue] };
+        let range: NSRange = unsafe { objc2::msg_send![&val, rangeValue] };
         assert_eq!(range, NSRange::from(1..2));
         // NSValue -getValue is broken on GNUStep for some types
         #[cfg(not(feature = "gnustep-1-7"))]
diff --git a/objc2/CHANGELOG.md b/objc2/CHANGELOG.md
@@ -38,6 +38,26 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
   `ClassBuilder::add_method`.
 * Renamed `ClassDecl` and `ProtocolDecl` to `ClassBuilder` and
   `ProtocolBuilder`. The old names are kept as deprecated aliases.
+* **BREAKING**: Changed how `msg_send!` works wrt. capturing its arguments.
+
+  This will require changes to your code wherever you used `Id`, for example:
+  ```rust
+  // Before
+  let obj: Id<Object, Owned> = ...;
+  let p: i32 = unsafe { msg_send![obj, parameter] };
+  let _: () = unsafe { msg_send![obj, setParameter: p + 1] };
+  // After
+  let mut obj: Id<Object, Owned> = ...;
+  let p: i32 = unsafe { msg_send![&obj, parameter] };
+  let _: () = unsafe { msg_send![&mut obj, setParameter: p + 1] };
+  ```
+
+  Notice that we now clearly pass `obj` by reference, and therein also
+  communicate the mutability of the object (in the first case, immutable, and
+  in the second, mutable).
+
+  If you previously used `*mut Object` or `&Object` as the receiver, message
+  sending should work exactly as before.
 
 ### Fixed
 * Properly sealed the `MessageArguments` trait (it already had a hidden
diff --git a/objc2/examples/introspection.rs b/objc2/examples/introspection.rs
@@ -42,6 +42,6 @@ fn main() {
     }
 
     // Invoke a method on the object
-    let hash: usize = unsafe { msg_send![obj, hash] };
+    let hash: usize = unsafe { msg_send![&obj, hash] };
     println!("NSObject hash: {}", hash);
 }
diff --git a/objc2/examples/talk_to_me.rs b/objc2/examples/talk_to_me.rs
@@ -31,9 +31,9 @@ fn main() {
     let utterance: *mut Object = unsafe { msg_send![utterance, initWithString: &*string] };
     let utterance: Id<Object, Owned> = unsafe { Id::new(utterance).unwrap() };
 
-    // let _: () = unsafe { msg_send![utterance, setVolume: 90.0f32 };
-    // let _: () = unsafe { msg_send![utterance, setRate: 0.50f32 };
-    // let _: () = unsafe { msg_send![utterance, setPitchMultiplier: 0.80f32 };
+    // let _: () = unsafe { msg_send![&utterance, setVolume: 90.0f32 };
+    // let _: () = unsafe { msg_send![&utterance, setRate: 0.50f32 };
+    // let _: () = unsafe { msg_send![&utterance, setPitchMultiplier: 0.80f32 };
 
-    let _: () = unsafe { msg_send![synthesizer, speakUtterance: &*utterance] };
+    let _: () = unsafe { msg_send![&synthesizer, speakUtterance: &*utterance] };
 }
diff --git a/objc2/src/declare.rs b/objc2/src/declare.rs
@@ -415,9 +415,9 @@ mod tests {
     #[test]
     fn test_custom_class() {
         // Registering the custom class is in test_utils
-        let obj = test_utils::custom_object();
-        let _: () = unsafe { msg_send![obj, setFoo: 13u32] };
-        let result: u32 = unsafe { msg_send![obj, foo] };
+        let mut obj = test_utils::custom_object();
+        let _: () = unsafe { msg_send![&mut obj, setFoo: 13u32] };
+        let result: u32 = unsafe { msg_send![&obj, foo] };
         assert_eq!(result, 13);
     }
 
diff --git a/objc2/src/lib.rs b/objc2/src/lib.rs
@@ -42,8 +42,8 @@
 //! };
 //!
 //! // Usage
-//! let hash: NSUInteger = unsafe { msg_send![obj, hash] };
-//! let is_kind = unsafe { msg_send_bool![obj, isKindOfClass: cls] };
+//! let hash: NSUInteger = unsafe { msg_send![&obj, hash] };
+//! let is_kind = unsafe { msg_send_bool![&obj, isKindOfClass: cls] };
 //! assert!(is_kind);
 //! ```
 //!
diff --git a/objc2/src/macros.rs b/objc2/src/macros.rs
@@ -54,22 +54,32 @@ macro_rules! sel {
 
 /// Sends a message to an object or class.
 ///
-/// The first argument can be any type that implements [`MessageReceiver`],
-/// like a reference, a pointer, or an [`rc::Id`] to an object (where the
-/// object implements [`Message`]).
-///
-/// In general this is wildly `unsafe`, even more so than sending messages in
+/// This is wildly `unsafe`, even more so than sending messages in
 /// Objective-C, because this macro doesn't know the expected types and
 /// because Rust has more safety invariants to uphold. Make sure to review the
-/// safety section below.
+/// safety section below!
+///
+/// # General information
+///
+/// The syntax is similar to the message syntax in Objective-C, except we
+/// allow an optional comma between arguments (works better with rustfmt).
 ///
-/// The syntax is similar to the message syntax in Objective-C.
+/// The first argument (know as the "receiver") can be any type that
+/// implements [`MessageReceiver`], like a reference or a pointer to an
+/// object, or even a reference to an [`rc::Id`] containing an object.
+/// Each subsequent argument must implement [`Encode`].
+///
+/// Behind the scenes this translates into a call to [`sel!`], and afterwards
+/// a fully qualified call to [`MessageReceiver::send_message`] (note that
+/// this means that auto-dereferencing of the receiver is not supported,
+/// making the ergonomics when using this slightly worse).
 ///
 /// Variadic arguments are not currently supported.
 ///
 /// [`MessageReceiver`]: crate::MessageReceiver
-/// [`Message`]: crate::Message
 /// [`rc::Id`]: crate::rc::Id
+/// [`Encode`]: crate::Encode
+/// [`MessageReceiver::send_message`]: crate::MessageReceiver::send_message
 ///
 /// # Panics
 ///
@@ -83,13 +93,12 @@ macro_rules! sel {
 ///
 /// # Safety
 ///
-/// The user must ensure that the selector is a valid method and is available
-/// on the given receiver.
+/// This macro can't inspect header files to see the expected types, so it is
+/// your responsibility that the selector exists on the receiver, and that the
+/// argument types and return type are what the receiver excepts for this
+/// selector - similar to defining an external function in FFI.
 ///
-/// Since this macro can't inspect header files to see the expected types, it
-/// is the users responsibility that the argument types and return type are
-/// what the receiver excepts for this selector. A way of doing this is by
-/// defining a wrapper function:
+/// The recommended way of doing this is by defining a wrapper function:
 /// ```
 /// # use std::os::raw::{c_int, c_char};
 /// # use objc2::msg_send;
@@ -99,19 +108,35 @@ macro_rules! sel {
 /// }
 /// ```
 ///
-/// The user must also uphold any safety requirements (explicit and implicit)
-/// that the method has (e.g. methods that take pointers as an argument
-/// usually require that the pointer is valid and often non-null).
+/// This way we are clearly communicating to Rust that this method takes an
+/// immutable object, a C-integer, and returns a pointer to (probably) a
+/// C-compatible string. Afterwards, it becomes fairly trivial to make a safe
+/// abstraction around this.
+///
+/// In particular, you must uphold the following requirements:
+///
+/// 1. The selector is a valid method that is available on the given receiver.
+///
+/// 2. The types of the receiver and arguments must match what is expected on
+///   the Objective-C side.
+///
+/// 3. The call must not violate Rust's mutability rules, e.g. if passing an
+///   `&T`, the Objective-C method must not mutate the variable (this is true
+///   for receivers as well).
+///
+/// 4. If the receiver is a raw pointer the user must ensure that it is valid
+///   (aligned, dereferenceable, initialized and so on). Messages to `null`
+///   pointers are allowed (though heavily discouraged), but only if the
+///   return type itself is a pointer.
 ///
-/// Additionally, the call must not violate Rust's mutability rules, e.g. if
-/// passing an `&T` the Objective-C method must not mutate the variable.
+/// 5. The method must not (yet, see [RFC-2945]) throw an exception.
 ///
-/// If the receiver is a raw pointer the user must ensure that it is valid
-/// (aligned, dereferenceable, initialized and so on). Messages to `null`
-/// pointers are allowed (though discouraged), but only if the return type
-/// itself is a pointer.
+/// 6. You must uphold any additional safety requirements (explicit and
+///   implicit) that the method has (for example, methods that take pointers
+///   usually require that the pointer is valid, and sometimes non-null.
+///   Another example, some methods may only be called on the main thread).
 ///
-/// Finally, the method must not (yet, see [RFC-2945]) throw an exception.
+/// 7. TODO: Maybe more?
 ///
 /// # Examples
 ///
@@ -132,7 +157,7 @@ macro_rules! msg_send {
     [super($obj:expr, $superclass:expr), $selector:ident $(,)?] => ({
         let sel = $crate::sel!($selector);
         let result;
-        match $crate::MessageReceiver::send_super_message(&$obj, $superclass, sel, ()) {
+        match $crate::MessageReceiver::send_super_message($obj, $superclass, sel, ()) {
             Err(s) => panic!("{}", s),
             Ok(r) => result = r,
         }
@@ -141,7 +166,7 @@ macro_rules! msg_send {
     [super($obj:expr, $superclass:expr), $($selector:ident : $argument:expr $(,)?)+] => ({
         let sel = $crate::sel!($($selector :)+);
         let result;
-        match $crate::MessageReceiver::send_super_message(&$obj, $superclass, sel, ($($argument,)+)) {
+        match $crate::MessageReceiver::send_super_message($obj, $superclass, sel, ($($argument,)+)) {
             Err(s) => panic!("{}", s),
             Ok(r) => result = r,
         }
@@ -150,7 +175,7 @@ macro_rules! msg_send {
     [$obj:expr, $selector:ident $(,)?] => ({
         let sel = $crate::sel!($selector);
         let result;
-        match $crate::MessageReceiver::send_message(&$obj, sel, ()) {
+        match $crate::MessageReceiver::send_message($obj, sel, ()) {
             Err(s) => panic!("{}", s),
             Ok(r) => result = r,
         }
@@ -159,7 +184,7 @@ macro_rules! msg_send {
     [$obj:expr, $($selector:ident : $argument:expr $(,)?)+] => ({
         let sel = $crate::sel!($($selector :)+);
         let result;
-        match $crate::MessageReceiver::send_message(&$obj, sel, ($($argument,)+)) {
+        match $crate::MessageReceiver::send_message($obj, sel, ($($argument,)+)) {
             Err(s) => panic!("{}", s),
             Ok(r) => result = r,
         }
diff --git a/objc2/src/message/mod.rs b/objc2/src/message/mod.rs
diff --git a/objc2/src/test_utils.rs b/objc2/src/test_utils.rs
diff --git a/tests/assembly/test_msg_send_zero_cost/lib.rs b/tests/assembly/test_msg_send_zero_cost/lib.rs
diff --git a/tests/assembly/test_retain_autoreleased/lib.rs b/tests/assembly/test_retain_autoreleased/lib.rs
diff --git a/tests/ui/msg_send_only_message.stderr b/tests/ui/msg_send_only_message.stderr

Original file line number	Diff line number	Diff line change
`@@ -70,12 +70,12 @@ fn main() {`
`70`	`70`
`71`	`71`	`obj.set_number(7);`
`72`	`72`	`println!("Number: {}", unsafe {`
`73`		`- let number: u32 = msg_send![obj, number];`
	`73`	`+ let number: u32 = msg_send![&obj, number];`
`74`	`74`	`number`
`75`	`75`	`});`
`76`	`76`
`77`	`77`	`unsafe {`
`78`		`- let _: () = msg_send![obj, setNumber: 12u32];`
	`78`	`+ let _: () = msg_send![&mut obj, setNumber: 12u32];`
`79`	`79`	`}`
`80`	`80`	`println!("Number: {}", obj.number());`
`81`	`81`	`}`
Original file line number	Diff line number	Diff line change
`@@ -129,7 +129,7 @@ impl<K: Message, V: Message> NSDictionary<K, V> {`
`129`	`129`
`130`	`130`	`pub fn into_values_array(dict: Id<Self, Owned>) -> Id<NSArray<V, Owned>, Shared> {`
`131`	`131`	`unsafe {`
`132`		`- let vals = msg_send![dict, allValues];`
	`132`	`+ let vals = msg_send![&dict, allValues];`
`133`	`133`	`Id::retain_autoreleased(vals).unwrap()`
`134`	`134`	`}`
`135`	`135`	`}`
Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,7 @@ impl<'a, T: Message> Iterator for NSEnumerator<'a, T> {`
`33`	`33`	`type Item = &'a T;`
`34`	`34`
`35`	`35`	`fn next(&mut self) -> Option<&'a T> {`
`36`		`- unsafe { msg_send![self.id, nextObject] }`
	`36`	`+ unsafe { msg_send![&mut self.id, nextObject] }`
`37`	`37`	`}`
`38`	`38`	`}`
`39`	`39`
Original file line number	Diff line number	Diff line change
`@@ -42,6 +42,6 @@ fn main() {`
`42`	`42`	`}`
`43`	`43`
`44`	`44`	`// Invoke a method on the object`
`45`		`- let hash: usize = unsafe { msg_send![obj, hash] };`
	`45`	`+ let hash: usize = unsafe { msg_send![&obj, hash] };`
`46`	`46`	`println!("NSObject hash: {}", hash);`
`47`	`47`	`}`