Implement Ref::{try_as_ref,try_into_ref,try_into_mut}

[jswrenn]

Only Ref::try_as_mut remains missing, probably pending polonius landing in rustc.

Partially fixes #1865
Supersedes #1184

A restricted form of Ref::try_as_mut can be implemented now as:

impl<B, T> Ref<B, T>
where
    B: ByteSlice,
    T: KnownLayout + ?Sized,
{
    #[must_use = "has no side effects"]
    #[inline(always)]
    pub fn try_as_mut(r: &mut Self) -> Result<&mut T, ValidityError<&mut Self, T>>
    where
        T: TryFromBytes + Immutable,
        B: ByteSliceMut + CloneableByteSlice,
    {
        // Presumably unreachable, since we've guarded each constructor of `Ref`.
        static_assert_dst_is_not_zst!(T);

        // Due to a false-positive in Rust's current borrow checker, the same
        // `r` can't be used for both validation *and* be returned upon
        // validation failure. We use `r_tmp` for validation, and `r` as the
        // value returned upon failure.
        let mut r_tmp = r.clone();

        // SAFETY: We don't call any methods on `t` other than those provided by
        // `ByteSlice`.
        let b = unsafe { r_tmp.as_byte_slice_mut() };

        match Ptr::from_mut(b.deref_mut()).try_cast_into_no_leftover::<T, BecauseExclusive>(None) {
            Ok(candidate) => match candidate.try_into_valid() {
                Ok(valid) => {
                    // SAFETY: `valid` inherits the local lifetime of `r_tmp`,
                    // but its value is derived from `r_tmp`'s underlying bytes.
                    // By contract on `CloneableByteSlice`, `r_temp.deref()
                    // produces a byte slice whose address (and thus lifetime)
                    // is identicial to that produced by `r.deref()`. We can
                    // therefor soundly extend the lifetime of `valid` to that
                    // of `r`.
                    Ok(unsafe { valid.assume_lifetime() }.as_mut())
                },
                Err(e) => Err(e.with_src(r)),
            },
            Err(CastError::Validity(i)) => match i {},
            Err(CastError::Alignment(_) | CastError::Size(_)) => {
                // SAFETY: By invariant on `Ref::0`, the referenced byte slice
                // is aligned to `T`'s alignment and its size corresponds to a
                // valid size for `T`. Since properties are checked upon
                // constructing `Ref`, these failures are unreachable.
                unsafe { core::hint::unreachable_unchecked() }
            }
        }
    }
}

...but we don't yet implement ClonableByteSlice for anything except &[u8], so the method is effectively untestable.

[joshlf]

This wigs me out. Maybe we can just use .into_byte_slice() and then add an unsafe Ref constructor that reconstitutes the Ref from the byte slice? That way we can use it to reconstruct the Ref in the error case and we don't have to do lifetime shenanigans.

[jswrenn]

I don't think that's possible without changing the return type — B is convertible into a &[u8], but it's not necessarily itself a &[u8].

Why does this wig you out? Lifetimes are fundamentally about the live extent of a referent, and our safety conditions on ByteSlice and IntoByteSlice are all about having a stable referent.

[joshlf]

Well r only lives as long as the body, so bytes's lifetime also doesn't outlive the body. I grant that the underlying allocation we're pointing to lives for 'a, but are we guaranteed that Rust won't make assumptions on the basis that r becomes unreachable after this scope ends? I'm not sure.

[compiler-errors]

Rust will never make optimizations based off of lifetimes because we specifically only computer lower-bound lifetimes in MIR and also erase them by the time we get to codegen.

[joshlf]

PoC of unsoundness: add the following to the bottom of this file and run ./cargo.sh +nightly miri test unsound --features alloc:

#[cfg(test)]
#[cfg(feature = "alloc")]
mod unsound_demo {
    use alloc::boxed::Box;

    use super::*;

    #[test]
    fn test_try_into_ref_unsound() {
        unsafe impl ByteSlice for Box<[u8]> {}

        unsafe impl<'a> IntoByteSlice<'a> for Box<[u8]> {
            fn into_byte_slice(self) -> &'a [u8] {
                Box::leak(self)
            }
        }

        let b: Box<[u8]> = Box::new([0u8; 1]);
        let r = Ref::<_, bool>::from_bytes(b).unwrap();
        let b = Ref::try_into_ref(r).unwrap();
        assert_eq!(b, &false);
    }
}

The problem is that we're using the normal Deref here instead of going through IntoByteSlice::into_byte_slice, and so we're dropping the B: ByteSlice before returning. For B types which have ownership semantics, this can cause unsoundness like it does here.

[joshlf]

Same here as above

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 94.87179% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.72%. Comparing base (0bee231) to head (311a259).
Report is 3 commits behind head on v0.8.x.

Files with missing lines	Patch %	Lines
src/ref.rs	94.59%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           v0.8.x    #1930      +/-   ##
==========================================
+ Coverage   87.59%   87.72%   +0.13%     
==========================================
  Files          16       16              
  Lines        5988     6103     +115     
==========================================
+ Hits         5245     5354     +109     
- Misses        743      749       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.