Memory leaks when passing Drop objects to msg_send!

[scoopr]

This is further investigation on SSheldon/rust-objc-foundation#9
I made an additional test to rust-objc that shows the issue here scoopr@b637c96.
It has a minimal wrapper around the type that implements Drop, and then when used as an argument to msg_send!, it seems the drop is then never called. Using it just as a receiver for a method call does seem to work as expected.

I'm still not sure if the wrapper is wrong, or if there is a bug in msg_send!, or something else, but the fact is that there already is bunch of code that expects the drop to happen, and it would be really nice to have a solution without leaks :)

[SSheldon]

Thanks for the example @scoopr!

So let's look at this part of the example:

let obj = test_utils::WrappedObject::new();
unsafe { msg_send![..., isEqual:obj]; }

Rust and the objc crate have no idea what types are supposed to be passed to isEqual:, so it just uses the types given. In this case the type given for the first argument is WrappedObject. So behind the scenes the msg_send! macro converts this down to an external function call of a function that effectively looks like this:

unsafe extern fn isEqual(self: *mut Object, cmd: Sel, object: WrappedObject)

For a function with this signature, the WrappedObject will be moved into the function and the function takes ownership, meaning that the caller no longer owns the object and cannot drop it. So basically we're expecting isEqual to drop the object for us.

But isEqual won't do that; it's an Objective-C method implementation and it knows nothing about Rust objects or how to drop them. So instead the object is never dropped and will be leaked.

Another potential danger here is that the Objective-C method implementation is expecting a pointer to an object, and instead we're giving it a Rust struct. In this case it happens that the layout of the Rust struct is exactly a pointer, but if it has any other fields, we could be in trouble.

So I'd say when writing this unsafe code, we must ensure that we're not passing any non-repr(C) types and not moving in any type that implements Drop.

To avoid this, you can write your code like:

msg_send![..., isEqual:&*obj]

Or, even better, you could write a little wrapper function that enforces the types you expect:

impl WrappedObjectRef {
    unsafe fn isEqual(&self, object: &WrappedObjectRef) -> BOOL {
        msg_send![self, isEqual:object]
    }
}

With this wrapper function, the code would report:

expected reference, found struct WrappedObject
help: consider borrowing here: &obj

I'm trying to think if there's ways the objc crate can make it harder to have this error:

• auto-dereferencing arguments? I'm not sure that's possible and it could have surprising results
• enforcing that all message arguments are Copy? Could make sense and would catch this issue, but can't solve the general issue of passing the wrong inferred type.

For now though this is one of the things that needs to be manually audited when writing unsafe code without the usual compiler protections.

Does that all make sense and solve your issue? Let me know if you've got any other ideas about this.

[scoopr]

I would very much like that there would be some enforcement that leaks wouldn't be quite as easy. I mean, rust-objc-foundation wrappers leak too if used in msg_send! arguments, and it isn't clear to me how they should be used instead.

First, I'm still not understanding what happens to the arguments. They are captured as.. tuples? which then implement MessageArguments trait? Why doesn't the tuple drop the contents at the end of the invocation?

One other solution I was thinking was having the Message trait have a method which would return the runtime::Object pointer, maybe have some default impl. Then it would be fairly unambiguous how a random type could be used as objc objects. But I guess that wouldn't necessary enforce the misuse by itself, but would be easy to fix in the macros that generate the wrappers.

I can't quite wrap my head around what kinds of dereferencing or inferring would/do happen in all this, so it may be I don't see the forest from the trees here.

[kvark]

It sounds to me that Copy bound addresses this concern rigidly, even though I share @scoopr inquiry about why the tuple doesn't get dropped in the first place...

can't solve the general issue of passing the wrong inferred type.

Perhaps, we can solve one thing at a time?