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

Unsoundness in OwningRef::map_with_owner and more #77

Open
noamtashma opened this issue Jan 23, 2022 · 2 comments · May be fixed by #78
Open

Unsoundness in OwningRef::map_with_owner and more #77

noamtashma opened this issue Jan 23, 2022 · 2 comments · May be fixed by #78

Comments

@noamtashma
Copy link

I found more unsoundness problems. This extends #61 .

In particular, OwningRef::map_with_owner allows creating an OwningRef that points at the owner, which can be moved.

fn unstable_address() {
    let ow_ref = OwningRef::new(Box::new(5));
    let new_ow_ref = ow_ref.map_with_owner(|owner, _my_ref| owner);
    println!("Reading memory that was moved from: {}", *new_ow_ref);
}

This by itself can be fixed by replacing map_with_owner with a method that only gives out a reference to the referent of the owner, like this:

pub fn map_with_owner<F, U: ?Sized>(self, f: F) -> OwningRef<'t, O, U>
    where O: StableAddress + Deref,
        F: for<'a> FnOnce(&'a O::Target, &'a T) -> &'a U

In addition, There's unsoundness combining a conversion from OwningRefMut to OwningRef together with methods that can read the owner of an OwningRef, like so:

fn ref_mut_to_ref() {
    use core::cell::RefCell; // `Cell` and any other kind of cell also works
    let ow_ref = OwningRefMut::new(Box::new(RefCell::new(Box::new(5))));
    let new_ow_ref = ow_ref.map(|cell| &**cell.get_mut());
    // Have to convert to `OwningRef` In order to use `OwningRef::as_owner`
    // instead of `OwningRefMut::as_owner`.
    *new_ow_ref.as_owner().borrow_mut() = Box::new(9);
    println!("Reading deallocated memory: {}", *new_ow_ref);
}

OwningRefMut::{as_owner, as_owner_mut} can also be used, as #61 shows.

There are two ways to fix this, and each choice corresponds to a small difference in the meaning and invariants of OwningRef:

  • Disallow conversions from OwningRefMut to OwningRef, and allow shared access to the owner of an OwningRef.
    Invariant: the reference of the OwningRef may only borrow immutably from the owner.
  • Allow conversions from OwningRefMut to OwningRef, but disallow shared access to the owner of an OwningRef.
    Invariant: the reference of the OwningRef may borrow immutably or mutably from the owner.

Essentially, these are two distinct types, which are both sound by themselves, and a third option is to have both types.

@noamtashma noamtashma linked a pull request Jan 23, 2022 that will close this issue
@slonik-az
Copy link

This repo has not received any commits for two years (since Feb 27, 2020) and seems unmaintained. Did you try to contact the owners directly?

@noamtashma
Copy link
Author

Hey everyone, since so long has passed and still the maintainer hasn't showed up and no one made a replacement crate, I decided to make my pull request into a crate.

It's available as safer_owning_ref.

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

Successfully merging a pull request may close this issue.

2 participants