Soundness issue with `msg_send!`

[madsmtm]

So, during my work on objc2 I decided to test some things in Miri, to try to get a feel for how sound everything is, and found this issue (which is currently present in both crates).

Below is a piece of code that tries to mutate an ivar on an object, resembling fairly closely something that is quite commonly done in winit. Try placing it under objc/examples/msg_send_unsound.rs, and running cargo miri run --example msg_send_unsound.

#[macro_use]
extern crate objc; // v0.2.7

use objc::runtime::{Object, Sel};

// Assume this is registered to the object with `ClassDecl::add_method`
extern "C" fn my_selector(obj: *mut Object, _sel: Sel) {
    let obj = unsafe { &mut *obj };
    let a = unsafe { obj.get_mut_ivar::<i32>("a") };
    *a += 1;
}

fn main() {
    let ptr: *mut Object = new_object(42); // ivar a = 42
    let obj: &mut Object = unsafe { &mut *ptr };

    // Get an immutable reference to the instance variable
    let a = unsafe { obj.get_ivar::<i32>("a") };

    unsafe {
        // Uses `obj` mutably, but the signature says it's used immutably
        let _: () = msg_send![obj, my_selector];
    }

    // So the compiler can't catch that we're not allowed to access `a` here!
    assert_eq!(*a, 43);

    free_object(ptr);
}

// ------------------------------------
//
// HACKY STUBS BELOW TO MAKE MIRI WORK!
//
// ------------------------------------

use std::ffi::CStr;
use std::os::raw::c_char;
use std::ptr;

use objc::runtime::{Class, Ivar};

#[repr(C)]
struct MyObject {
    isa: *const Class,
    a: i32,
}

fn new_object(a: i32) -> *mut Object {
    let obj = Box::new(MyObject {
        isa: ptr::null(),
        a,
    });
    Box::into_raw(obj) as *mut Object
}

fn free_object(obj: *mut Object) {
    unsafe { Box::from_raw(obj as *mut MyObject) };
}

#[no_mangle]
extern "C" fn sel_registerName(name: *const c_char) -> Sel {
    unsafe { Sel::from_ptr(name.cast()) }
}

#[no_mangle]
extern "C" fn objc_msgSend(obj: *mut Object, sel: Sel) {
    my_selector(obj, sel)
}

#[no_mangle]
extern "C" fn object_getClass(obj: *const Object) -> *const Class {
    // Must be a valid pointer, so don't return isa
    obj.cast()
}

#[no_mangle]
extern "C" fn class_getInstanceVariable(cls: *const Class, _name: *const c_char) -> *const Ivar {
    cls.cast()
}

#[no_mangle]
extern "C" fn ivar_getTypeEncoding(_ivar: *const Ivar) -> *const c_char {
    CStr::from_bytes_with_nul(b"i\0").unwrap().as_ptr()
}

#[no_mangle]
extern "C" fn ivar_getOffset(_ivar: *const Ivar) -> isize {
    // isa is 64 bits
    8
}

You should see the following miri error, which makes sense as explained in the code comments:

error: Undefined Behavior: trying to reborrow <6300> for SharedReadOnly permission at alloc1621[0x8], but that tag does not exist in the borrow stack for this location
   --> examples/msg_send_unsound.rs:26:5
    |
26  |     assert_eq!(*a, 43);
    |     ^^^^^^^^^^^^^^^^^^
    |     |
    |     trying to reborrow <6300> for SharedReadOnly permission at alloc1621[0x8], but that tag does not exist in the borrow stack for this location
    |     this error occurs as part of a reborrow at alloc1621[0x8..0xc]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <6300> was created by a retag at offsets [0x8..0xc]
   --> examples/msg_send_unsound.rs:18:22
    |
18  |     let a = unsafe { obj.get_ivar::<i32>("a") };
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^
help: <6300> was later invalidated at offsets [0x8..0xc]
   --> src/runtime.rs:510:9
    |
510 |         &mut *ptr
    |         ^^^^^^^^^
    = note: inside `main` at $TOOLCHAIN/lib/rustlib/src/rust/library/core/src/macros/mod.rs:38:16
    = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)

A bit more info

The problem is that you have to somehow specify that the object is mutated by the message send, which you can't really do properly.

Just to illustrate that this is indeed an issue with how msg_send! works, try applying the following quick patch which changes msg_send! to always require mutable objects (obviously not a real fix):

diff --git a/src/macros.rs b/src/macros.rs
--- a/src/macros.rs
+++ b/src/macros.rs
@@ -130,7 +130,7 @@ macro_rules! msg_send {
     ($obj:expr, $name:ident) => ({
         let sel = sel!($name);
         let result;
-        match $crate::__send_message(&*$obj, sel, ()) {
+        match $crate::__send_message(&mut *$obj, sel, ()) {
             Err(s) => panic!("{}", s),
             Ok(r) => result = r,
         }
@@ -139,7 +139,7 @@ macro_rules! msg_send {
     ($obj:expr, $($name:ident : $arg:expr)+) => ({
         let sel = sel!($($name:)+);
         let result;
-        match $crate::__send_message(&*$obj, sel, ($($arg,)*)) {
+        match $crate::__send_message(&mut *$obj, sel, ($($arg,)*)) {
             Err(s) => panic!("{}", s),
             Ok(r) => result = r,
         }

Then you'll get the expected rustc error:

error[E0502]: cannot borrow `*obj` as mutable because it is also borrowed as immutable
  --> examples/msg_send_unsound.rs:22:21
   |
18 |     let a = unsafe { obj.get_ivar::<i32>("a") };
   |                      ------------------------ immutable borrow occurs here
...
22 |         let _: () = msg_send![obj, my_selector];
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
...
26 |     assert_eq!(*a, 43);
   |     ------------------ immutable borrow later used here
   |
   = note: this error originates in the macro `msg_send` (in Nightly builds, run with -Z macro-backtrace for more info)

This is similar to the problem with nulls I noted in https://github.com/SSheldon/rust-objc/pull/102, and probably requires some kind of change in msg_send!, though I don't actually know how to do this yet (suggestions appreciated).

[madsmtm]

Idea 1

Just wrap all your message sends in a new function:

fn call_my_selector(obj: &mut Object) {
    unsafe { msg_send![obj, my_selector] }
}

Which, well, is probably a good idea anyway because of the difficulties with determining the type of input and return types. Still not entirely sure this is sound though, since we're effectively turning an &Object (created inside msg_send!) into an &mut Object, but miri doesn't complain about it (as far as I understand because Object is a ZST, and those have special rules) so maybe it is?

Idea 2

Add msg_send_mut! which requires that the object is mutable? Or maybe msg_send![mut obj, my_selector]?

Idea 3

Pass the object in msg_send! directly to Message::send_message (without taking a reference to it first), and instead require the object to be one of the following types (where T: Message):

• *const T
• *mut T
• &T
• &mut T
• &Id<T, O: Ownership>
• &mut Id<T, Owned>
• ...

(Which would unfortunately require message sends to Id to be called with msg_send![&obj], which is not very ergonomic)

[SSheldon]

@madsmtm would it help anything if we stopped trying to have &/&mut references to Object? I've been thinking that in practice it's probably never truly safe to have a Rust reference to an Objective-C object given how strict Rust's reference rules are and how lax Objective-C is. Like, autorelease an object? Well, somewhere now an autorelease pool is probably holding onto a pointer to the object and will mutate its retain count later, and this means technically an &mut reference to it is invalid.

Maybe instead of trying to treat pointers to Objective-C objects like Rust references, we should just be honest about the safety and have more unsafe and take *mut Object everywhere. get_ivar could take an object pointer and return a pointer and leave it to the developer to determine when it's safe to dereference or not.

Or, less drastic but maybe we should at least have get_mut_ivar take &self and return *mut T (and probably change get_ivar to also return a pointer for consistency).

[madsmtm]

A first step would be to add UnsafeCell somewhere in https://github.com/SSheldon/rust-objc/blob/0.2.7/src/runtime.rs#L43 or similar places - that would as far as I know make it so that internal things that Object may hold are allowed to be mutated (of course ideally we'd just mark Object as an extern type, but that's beside the point).

get_mut_ivar take &self and return *mut T

I think this would now behave similar to UnsafeCell::get, which would probably be fine (though of course that still doesn't save the user from creating two overlapping &mut T, but UnsafeCell doesn't do that either). Could improve this using ideas from Cell::get and Cell::set.

autorelease an object? Well, somewhere now an autorelease pool is probably holding onto a pointer to the object and will mutate its retain count later, and this means technically an &mut reference to it is invalid.

Huh, never considered that. Actually it may not even be possible to soundly implement autoreleasepools in Rust (under current Stacked Borrows) - I'll look into that at some point, if it is, then this is a non-issue, if not, it's probably an issue that the UCG WG would like to know about.

@madsmtm would it help anything if we stopped trying to have &/&mut references to Object?

I don't think this is a clear "either is better" situation - On one hand, doing this would ensure that objc is sound, on the other I think it would make it harder for downstream users to ensure that their code is sound.

A reasonable middle ground would maybe be to use &Object everywhere. E.g. an implementation of NSArray could be mutated using fn push(&self, obj: &Object) { msg_send![self, addObject: obj] }, and that would probably still be safe (as long as NSArray contains UnsafeCell). Of course, such an implementation wouldn't be thread safe, but that's probably not that important.

Anyhow, as you know, I'd like to see more (safe) Objective-C code in the wild, so I believe the trade-off in increased complexity on our end to accommodate this is worth it, but I recognize the very real possibility that I'll change my mind on this at some point.