Investigate possible unsafety of `OwningHandle`

[Kimundi]

See https://github.com/Kimundi/owning-ref-rs/pull/15#issuecomment-262142161

[Storyyeller]

My understanding is that the issue comes from the fake lifetime parameters of the handle. OwningHandle hides the handle value itself, but depending on the implementation of the handle, that is insufficient to prevent the fake lifetime from leaking into safe code. Unfortunately, there is no way to check this in generic code.

On the other hand, there is technically no unsoundness, because the only way to get a fake lifetime parameter in the first place is to perform a pointer deref in the callback passed to OwningHandle::new_with_fn, which requires unsafe code. On the other hand, that's pretty much the intended use case, so realistically, every caller will do so. I think the best option is to add more documentation explaining about how the API is dangerous and the caller is responsible for ensuring that fake lifetime parameters don't leak or aren't used unsafely.

The other place to watch out is ToHandle/ToHandleMut. Currently, the only implementation of ToHandle provided is for RefCell, and it does introduce a fake lifetime. However, my understanding is that the T: 'static bound on the impl prevents it from leaking. I could be missing something, but I don't see any unsafety offhand.

The other issue is that ToHandle is a safe, public trait. However, in order for users to implement it, they would have to perform a pointer deref, which requires unsafe code, so there is technically no unsoundness. Still, I think it might be a good idea to make ToHandle and ToHandleMut into unsafe traits so that people realize that they have to be extremely careful when implementing these traits.

[jpernst]

As @Storyyeller points out, the unsafety here is a result of the false lifetime being exposed to the outside world. The H: Deref bound was intended to prevent this by forcing you to reborrow the false lifetime away, but we can defeat that by just adding an extra level of deref. The false lifetime being exposed is very dangerous because borrowck can and will unify it with things that it shouldn't be unified with. Conceptually it's an existential lifetime known only to the OnwningHandle, but unfortunately there is no way to express that in rust yet.

After making that comment, I took some time to ponder this and if it was even possible to make this kind of thing safe at all. Rental was the result of that experimentation, but as can be seen from the code, it involves an absurdly arcane macro and subtle HRTB trickery to achieve it.

To boil it down, the only way to make it safe is to only allow access to the borrowed value within a closure bounded by: for<'a, 'fake> Fn(&'a MyHandle<'fake>), where 'fake is the false lifetime in this case. For a closure to satisfy this trait, it must be callable for any such lifetime, and since the lifetime that will be used is unknown in the context where the closure is created, borrowck can't unify it with anything outside of the closure, except in cases where it would be known to be variance-compatible anyway. Thus it becomes effectively existential for our purposes, and the code inside the closure is safe. Unfortunately, we can't express such a bound generically, since rust lacks HKT/ATC for the time being. This is what the gross macro is for, to construct a new type for each owner/borrow pair with the appropriate trait bounds.

I plan to redesign it as new enabling features land, but for now I can't envision a better way to expose an API like this. If you concede to allowing possible unsafety, though, the situation becomes much more tractable. One way would be to clearly mandate that the false lifetime must NOT appear in the type signature of the Deref target of the borrowed value. I don't think this can be mechanically enforced though. I do it in rental with a helper macro that detects the false lifetime token in a type signature and erases it, but I'm not sure if such a thing could be expressed with a trait or other static means.

[mitsuhiko]

There are some curious cases you can run into because of those faked lifetimes. In particular it permits you to crate code that looks okay but only becomes visible unsafe when you do things somewhere completely different.

In my case I have something that gives access to some mmap'ed or borrowed data. The mmapp'ed version is 'static. As a result of that, if not shielded carefully in a wrapper type you can easily give out borrows with a 'static lifetime instead of the lifetime of self.

I'm not sure what lesson is there to learn but right now I don't have much other options other than using OwnedHandle very carefully and wrapping it in another type that makes sure the exposed API is safe.