Soundness problems?

[saethlin]

I think there are a few problems with this crate, which or may not be soundness problems depending on how you squint.

1. This crate is intended to support owning_ref which has a few soundness problems, and rental which is debatably sound as well, and now unmaintained.

2. This crate provides an implementation of StableDeref for Box. Under Stacked Borrows (which is a prototype model and may not be adopted) this is technically true of Box, but useless. A pointer to the contents of a Box may point to the same address after the Box is moved, but said pointer is invalidated by the move and dereferencing it is UB. This is checked by Miri. This pattern may also be unsound for real, right now because rustc currently applies LLVM's noalias to Box. Users interested in an RAII heap allocation without the uniqueness property that Box has may be interested in aliasable.

3. I think the fact that &mut implements StableDeref suggests that you can write programs like this:

fn main() {
    let mut x = 1i32;
    let y = &mut x;
    let ptr = &*y as *const i32;
    drop(y);
    unsafe {
        println!("{}", *ptr);
    }
}

This program is similarly UB under Stacked Borrows, because moving the mutable reference retags it, invalidates all other pointers to its pointee. I don't know if this pattern is similarly unsound with respect to rustc's current use of LLVM noalias.

4. I think Document StableDeref for fat pointers #3 is a soundness problem. Again, it's a bit fuzzy to me exactly what the docs are supposed to permit, but they say this

More specifically, implementors must ensure that the result of calling deref() is valid for the lifetime of the object, not just the lifetime of the borrow, and that the deref is valid even if the object is moved. Also, it must be valid even after invoking arbitrary &self methods or doing anything transitively accessible from &Self.

Both Rc and Arc provide ::as_ptr, through which it is valid to do mutation. In my opinion, mutating through this pointer qualifies as "arbitrary &self methods or anything transitively accessible from &Self". But that permits mutating the referent of a &T while it is active, which is unsound.

[Storyyeller]

Regarding #3: StableDeref is only intended to guarantee validity as long as the parent object is not dropped. You could say the same thing about any type. E.g. taking a pointer to string data and then dropping the string will also result in a dangling pointer.

Regarding #4, it is not valid to mutate through a const pointer unless the target is inside an unsafecell. But even if it was, writing to pointers is unsafe, and anyone messing about with pointers knows what they are getting into.

As for the rest, I'm not sure what can be done in that case other than get rid of owning_ref entirely.

[saethlin]

StableDeref is only intended to guarantee validity as long as the parent object is not dropped

Sorry, my use of drop here was misleading. Any move will do. This program is rejected by Miri for exactly the same reason as my original example:

fn main() {
    let mut x = 1i32;
    let y = &mut x;
    let ptr = &*y as *const i32;
    let _z = y;
    unsafe {
        println!("{}", *ptr);
    }
}

it is not valid to mutate through a const pointer unless the target is inside an unsafecell

This program is accepted by Miri:

fn main() {
    let x = std::rc::Rc::new(1);
    unsafe {
        let ptr = std::rc::Rc::as_ptr(&x);
        core::ptr::write(ptr as *mut i32, 2);
    }
    println!("{:?}", x);
}

const vs mut on pointers is just a lint. The role of UnsafeCell is to permit mutation through pointers derived from &T, which is normally forbidden. The crux of my point is that this crate seems to assume that this guarantee extends to Deref impls, which it does not. Rc and Arc are both pointer wrappers. So a shared reference to the smart pointer can be used to obtain the contained raw pointer, which in Stacked Borrows terminology has SharedReadWrite permission to the inner T, and thus permits mutation of it.

But even if it was, writing to pointers is unsafe

True, but this is besides my point. This crate provides a StableDeref implementation for types which do not have a stable Deref implementation. In these cases, the reference produces from the Deref impl can be invalidated by access through a shared reference to the implementor. This crate documents that StableDeref is only implemented where this isn't possible. This is a contradiction, so either the docs for this crate are wrong, or it is wrong to provide these implementations.

and anyone messing about with pointers knows what they are getting into.

This is a tangent, but this statement is demonstrably false. For example, protobuf used to and rayon still does rely on indexing out of bounds with slice::get_unchecked_mut which is UB per the documentation. The authors would not have written such code if they knew what they were getting into.

[Storyyeller]

Yeah, I forgot that there's no const/mut pointer distinction in stacked borrows. Still, pointers require unsafe to use, so it's clearly out of bounds as far as the safety guarantees go.

Anyway, it seems like there are two main issues here

1. The treatment of Box as unique by Stacked Borrows is too strict and rules out usage that is common in the Rust ecosystem
2. owning_ref is unmaintained and has known soundness issues.

It doesn't seem like there's anything I can do about this, other than maybe add a warning telling people that they probably shouldn't use owning_ref and its ilk.

[saethlin]

pointers require unsafe to use, so it's clearly out of bounds as far as the safety guarantees go.

That's not how this works at all. From the nomicon:

The decision of whether to mark a trait unsafe is an API design choice. A safe trait is easier to implement, but any unsafe code that relies on it must defend against incorrect behavior. Marking a trait unsafe shifts this responsibility to the implementor.

Unsafe traits provide their guarantees even in the face of unsafe code, that's the whole point. When you're writing unsafe code, you can't rely on safe code doing what it documents. But you can rely on unsafe code doing what it documents (otherwise you can't rely on anything at all). No matter what unsafe code I write, so long as it isn't unsound on its own, I must be able to rely on the guarantees that this crate's unsafe traits make.

---

I think the problem with this crate is this wording:

Also, it must be valid even after invoking arbitrary &self methods or doing anything transitively accessible from &Self.

This just happened to remain correct until Rust 1.45, when Rc::as_ptr and Arc::as_ptr were stabilized. The receiver isn't important, it's what is actually done. The docs should probably say something like

Also, it must be valid even after invoking arbitrary &self methods or doing anything transitively accessible from &Self, so long as self and anything self owns is not mutated, even though interior mutability.

The core problem here is that this crate tries to make guarantees about types that it doesn't control. This crate relies on an undocumented property of another project, then advertises that property as a stable guarantee. It's great to provide an abstraction layer over the standard library's APIs that is is ever-cautious about providing its own traits for. But it's not great to do this by relying on undocumented properties of the types, especially properties that can be broken by adding features.

[peter-lyons-kehl]

Farther more, from source code of Vec it's clear that when resizing (either enlarging with .push(..), .extend_reserve(..), or downsizing with .shrink_to(..), .shrink_to_fit()theVec` implementation and/or the allocator being used are free to move the data to a different location (and update the pointer).