Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Violating soundness requirements #10

Open
SSheldon opened this issue Jan 19, 2020 · 1 comment
Open

Violating soundness requirements #10

SSheldon opened this issue Jan 19, 2020 · 1 comment

Comments

@SSheldon
Copy link
Owner

Treating Foundation objects as rust references is hard. Currently this crate can be used to violate soundness rules within safe code, which could result in undefined behavior.

One problem right now comes from NSCopying:

let mut string1 = NSString::from_str("Hello, world!");
let string2 = string1.copy();

let s1: &mut NSString = &mut string1;
let s2: &NSString = &string2;
println!("{:p}", s1);
println!("{:p}", s2);

This code results in an owned Id and a ShareId of the same object, allowing an &mut reference to an address while a & reference exists for the same address. This violates the aliasing requirements.

It may be the case that it's just too difficult to safely treat Objective-C objects as rust references and this crate is a failed experiment.

@madsmtm
Copy link

madsmtm commented Jun 3, 2021

As far as I can see, this is only a problem with the specific copy implementation, since it is essentially just a retain, which as you illustrate allows us to go from Id<T, Owned> -> &T -> Id<T, Owned>, and thereby violating the aliasing requirements.

In step 2 (&T -> Id<T, Owned>), the lifetime information is discarded - but if we preserved it, the problem would go away. For instance, it could be solved like this:

pub struct CopiedId<'a, T> {
    id: Id<T>,
    lifetime: PhantomData<&'a T>,
}

impl<'a, T> Deref<Target = T> for CopiedId<'a, T> {
    ...
}

pub trait INSCopying: INSObject {
    type Output: INSObject;
    fn copy<'a>(&'a self) -> CopiedId<'a, Self::Output> { }
}

Or alternatively, if this is a more common problem, we could change objc_id::Id to always take a lifetime parameter (that would just be 'static most of the time):

pub struct Id<'a, T, O = Owned> { }

I've outlined these to solve the general cases, but in this instance NSString is immutable, and hence there's never a need for Id<NSString, Owned> - so the from_str implementation could just return ShareId<NSString>, and the problem would be solved!

madsmtm added a commit to madsmtm/objc2 that referenced this issue Oct 4, 2021
Immutable types like NSString implement `copy` by simply retaining the pointer; hence, having an `Owned` NSString (which allows access to `&mut NSString`) is never valid, since it would be possible to create an aliased mutable reference.

So instead we now have an `Ownership` type on INSObject that indicates whether the type is mutable.

Fixes SSheldon/rust-objc-foundation#10 (see that issue for alternative ways to fix this).
madsmtm added a commit to madsmtm/objc2 that referenced this issue Oct 5, 2021
Immutable types like NSString implement `copy` by simply retaining the pointer; hence, having an `Owned` NSString (which allows access to `&mut NSString`) is never valid, since it would be possible to create an aliased mutable reference.

So instead we now have an `Ownership` type on INSObject that indicates whether the type is mutable.

Fixes SSheldon/rust-objc-foundation#10 (see that issue for alternative ways to fix this).
madsmtm added a commit to madsmtm/objc2 that referenced this issue Oct 30, 2021
Immutable types like NSString implement `copy` by simply retaining the pointer; hence, having an `Owned` NSString (which allows access to `&mut NSString`) is never valid, since it would be possible to create an aliased mutable reference.

So instead we now have an `Ownership` type on INSObject that indicates whether the type is mutable.

Fixes SSheldon/rust-objc-foundation#10 (see that issue for alternative ways to fix this).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants