StableDeref is unsound for &T

[KamilaBorowska]

The following code violates StableDeref requirements.

use stable_deref_trait::StableDeref;
use std::cell::Cell;
use std::ops::DerefMut;
use std::sync::atomic::{AtomicUsize, Ordering};

static POSITION: AtomicUsize = AtomicUsize::new(0);

struct MyEvilStruct<'a>([Cell<Option<&'a mut MyEvilStruct<'a>>>; 2]);

impl<'a> DerefMut for &MyEvilStruct<'a> {
    fn deref_mut(&mut self) -> &mut MyEvilStruct<'a> {
        self.0[POSITION.fetch_add(1, Ordering::Relaxed)].take().unwrap()
    }
}

fn assert_deref_mut_sane<T: StableDeref + DerefMut>(mut a: T) {
    let p: *mut T::Target = &mut *a;
    let q: *mut T::Target = &mut *a;
    assert_eq!(p, q);
}

fn main() {
    let mut a = MyEvilStruct([Cell::new(None), Cell::new(None)]);
    let mut b = MyEvilStruct([Cell::new(None), Cell::new(None)]);
    let p = MyEvilStruct([Cell::new(Some(&mut a)), Cell::new(Some(&mut b))]);
    assert_deref_mut_sane(&p);
}

[Storyyeller]

It sounds like you can supply a custom DerefMut impl for &T. Is that correct?

[KamilaBorowska]

Yes.

[Storyyeller]

That's unfortunate. What do you suggest? It seems like a fundamental assumption of owning_ref style crates is being violated, but there's no good way to fix it. Of course, there's already unsoundness due to noalias, but this might be easier to trigger.

[KamilaBorowska]

Honestly, I have no idea. There are two ways to go about it, but they are all to certain extent breaking changes.

1. Introduce new StableDerefMut trait that extends the StableDeref guarantee to DerefMut.
2. Remove impl<'a, T: ?Sized> StableDeref for &'a T. All other implementations seem fine. It's possible to introduce a newtype for references that is safe, unlike &T.

And there is a third way which isn't strictly speaking breaking, but is more complicated:

3. Get the compiler to prevent DerefMut implementations for &T. This would involve change in the language rather than this crate. Isn't too unlikely, as std::pin::Pin is affected by the same issue.

Waiting for standard library to fix the issue in std::pin::Pin may be acceptable as this issue affects standard library as well. While it is a "breaking change" according to RFC 1105 (&T is a fundamental type), I hope Rust developers will do it, because come on, who seriously implements DerefMut for shared references.

[Storyyeller]

I'll hope for the third option for now.

Option one or two risk breaking downstream users of owning_ref, etc.

[jdygert-spok]

For reference, this was fixed in Rust 1.45 here: https://github.com/rust-lang/rust/pull/68004/files#diff-00dbb2b0acb6b28a9dd6f1a9d31bf41095ad6da2b9fa2e322425db7bae31b2f1