Covariance in owner is problematic / can lead to unsoundness

[steffahn]

use self_cell::self_cell;

type X<'a> = &'a str;

self_cell! {
    pub struct Foo<'a> {
        owner: fn(&'a ()),
        #[covariant]
        dependent: X,
    }
}

fn transmute_lifetime<'a, 'b>(x: &'a str) -> &'b str {
    fn helper<'b>(s: &'b str) -> impl for<'a> FnOnce(&'a fn(&'b ())) -> &'a str {
        move |_| s
    }
    let x: Foo<'a> = Foo::new(|_| (), helper(x));
    let x: Foo<'static> = x; // coerce using variance 
    let y = Box::leak(Box::new(x));
    y.borrow_dependent()
}

fn main() {
    let r;
    {
        let s = "Hello World".to_owned();
        r = transmute_lifetime(s.as_str());
        dbg!(r); // "Hello World"
    }
    dbg!(r); // prints garbage :-)
}

[jrmuizel]

miri gives:

error: Undefined Behavior: type validation failed: encountered a dangling reference (use-after-free)
  --> src/main.rs:30:5
   |
30 |     dbg!(r); // prints garbage :-)
   |     ^^^^^^^^ type validation failed: encountered a dangling reference (use-after-free)
   |

[Voultapher]

Thanks for bringing this up! It's an unlucky combination of https://users.rust-lang.org/t/can-mut-u8-being-invariant-be-observed/60135 where I switched to NonNull which is covariant over it's owner, and then later I allowed lifetimes in the owner.

[steffahn]

Note that covariance in the owner isn’t necessarily per say unsound; so maybe a change to rule it out entirely is more harsh than necessary. I’m not quite done trying to understand the way ouroboros does this; they seem to be using a lifetime argument in place of the 'static placeholder lifetime (if any lifetime argument is present). And that seems to prevent the analogous code to the unsound example in this issue from compiling.

Another idea is that (at least as far as the exploit in this issue is concerned), only a contravariant owner is problematic, so in the case that the self_cell-struct has a lifetime, an additional marker field that’s PhantomData<&'a ()> where 'a is the lifetime argument, might be enough to make a contravariant owner type invariant.

[Voultapher]

With fn helper<'x>(s: &'x str) -> impl for<'z> FnOnce(&'z fn(&'x ())) -> &'z str as signature I'm not entirely sure I understand how s: &'x str is allowed to turn into &'z str. Shouldn't it enforce that the lifetime of &'z fn(&'x ()) and the thing that is returned is the same?

[steffahn]

I'll add an explanation of my understanding of why the code compiles shortly.

[Voultapher]

Did some experimentation and I can only get it to work for contravariant owners. And your suggested fix:

// marker to ensure that contravariant owners don't imply covariance
// over the dependent. See issue #18
owner_marker: PhantomData<$(&$OwnerLifetime)* ()>,

works like a charm and even gives nice error messages:

error[E0308]: mismatched types
   --> tests/self_cell.rs:533:31
    |
533 |         let x: Foo<'static> = x; // coerce using variance
    |                               ^ lifetime mismatch
    |
    = note: expected struct `owner_covariance::Foo<'static>`
               found struct `owner_covariance::Foo<'a>`
note: the lifetime `'a` as defined on the function body at 528:27...
   --> tests/self_cell.rs:528:27
    |
528 |     fn transmute_lifetime<'a, 'b>(x: &'a str) -> &'b str {
    |                           ^^
    = note: ...does not necessarily outlive the static lifetime

[steffahn]

owner_marker: PhantomData<$(&$OwnerLifetime)* ()>,

interesting approach xD; I would’ve put the entire field into the $(…)s.

Yeah, I guess that’s a good solution; just double-checked ouroboros, and it does seem like its support for contravariant types is effectively minimal and seems useless, so no major loss here :-)

[steffahn]

With fn helper<'x>(s: &'x str) -> impl for<'z> FnOnce(&'z fn(&'x ())) -> &'z str as signature I'm not entirely sure I understand how s: &'x str is allowed to turn into &'z str. Shouldn't it enforce that the lifetime of &'z fn(&'x ()) and the thing that is returned is the same?

I'll add an explanation of my understanding of why the code compiles shortly.

The important thing to note is that for<…>-quantified (i.e. higher order) lifetimes only quantify universally over those lifetimes that make the types involved still legal.

So

fn helper<'b>(s: &'b str) -> impl for<'a> FnOnce(&'a fn(&'b ())) -> &'a str

is effectively something like

fn helper<'b>(s: &'b str) -> impl for<'a, where 'b: 'a> FnOnce(&'a fn(&'b ())) -> &'a str

because without 'b: 'a, the type &'a fn(&'b ()) wouldn’t be legal

• &'a fn(&'b ())’s legality requires fn(&'b ()): 'a,
• which requires &'b (): 'a,
• which requires 'b: 'a

---

And that’s the problem with a contravariant lifetime in the owner. Your Foo::<'a>::new call only requires a
impl for<'this> FnOnce(&'this fn(&'a str)) -> &'a str
quantifying over 'this with 'a: 'this, while a Foo<'static> can outlive the lifetime 'a, and Foo<'static>::borrow_dependent can be called after 'a has ended, giving a &str that has lived longer than the upper bound of what the closure on creation of the Foo had to fulfill.

[Voultapher]

I plan on implementing this fix proper with testing soon.

[Voultapher]

I just pushed ff7d516, please let me know if this solves the issue. Once #19 is addressed I'll release a new version.

[steffahn]

Looks decent, I guess.