"Location is not frozen long enough" in bytes crate

[jrmuizel]

Here's a somewhat reduced version of the problem from the 'bytes' crate:

use std::{cmp, mem, usize};
use std::sync::atomic::{AtomicUsize, AtomicPtr};

pub struct BytesMut {
    inner: Inner,
}

#[cfg(target_endian = "little")]
#[repr(C)]
struct Inner {
    // WARNING: Do not access the fields directly unless you know what you are
    // doing. Instead, use the fns. See implementation comment above.
    arc: AtomicPtr<Shared>,
    ptr: *mut u8,
    len: usize,
    cap: usize,
}

struct Shared {
    vec: Vec<u8>,
    original_capacity_repr: usize,
    ref_count: AtomicUsize,
}

const MAX_ORIGINAL_CAPACITY_WIDTH: usize = 17;
// The original capacity algorithm will not take effect unless the originally
// allocated capacity was at least 1kb in size.
const MIN_ORIGINAL_CAPACITY_WIDTH: usize = 10;

#[cfg(target_pointer_width = "64")]
const INLINE_CAP: usize = 4 * 8 - 1;
#[cfg(target_pointer_width = "32")]
const INLINE_CAP: usize = 4 * 4 - 1;

// Buffer storage strategy flags.
const KIND_INLINE: usize = 0b01;
const KIND_VEC: usize = 0b11;
const KIND_MASK: usize = 0b11;

#[cfg(target_pointer_width = "64")]
const PTR_WIDTH: usize = 64;
#[cfg(target_pointer_width = "32")]
const PTR_WIDTH: usize = 32;

fn original_capacity_to_repr(cap: usize) -> usize {
    let width = PTR_WIDTH - ((cap >> MIN_ORIGINAL_CAPACITY_WIDTH).leading_zeros() as usize);
    cmp::min(width, MAX_ORIGINAL_CAPACITY_WIDTH - MIN_ORIGINAL_CAPACITY_WIDTH)
}

const ORIGINAL_CAPACITY_OFFSET: usize = 2;

impl Inner {

    #[inline]
    fn is_inline(&self) -> bool {
        self.kind() == KIND_INLINE
    }
    
    #[inline]
    fn kind(&self) -> usize {
        // This function is going to probably raise some eyebrows. The function
        // returns true if the buffer is stored inline. This is done by checking
        // the least significant bit in the `arc` field.
        //
        // Now, you may notice that `arc` is an `AtomicPtr` and this is
        // accessing it as a normal field without performing an atomic load...
        //
        // Again, the function only cares about the least significant bit, and
        // this bit is set when `Inner` is created and never changed after that.
        // All platforms have atomic "word" operations and won't randomly flip
        // bits, so even without any explicit atomic operations, reading the
        // flag will be correct.
        //
        // This function is very critical performance wise as it is called for
        // every operation. Performing an atomic load would mess with the
        // compiler's ability to optimize. Simple benchmarks show up to a 10%
        // slowdown using a `Relaxed` atomic load on x86.

        #[cfg(target_endian = "little")]
        #[inline]
        fn imp(arc: &AtomicPtr<Shared>) -> usize {
            unsafe {
                let p: &u8 = mem::transmute(arc);
                (*p as usize) & KIND_MASK
            }
        }

        #[cfg(target_endian = "big")]
        #[inline]
        fn imp(arc: &AtomicPtr<Shared>) -> usize {
            unsafe {
                let p: &usize = mem::transmute(arc);
                *p & KIND_MASK
            }
        }

        imp(&self.arc)
    }

    #[inline]
    fn capacity(&self) -> usize {
        if self.is_inline() {
            INLINE_CAP
        } else {
            self.cap
        }
    }

    #[inline]
    fn from_vec(mut src: Vec<u8>) -> Inner {
        let len = src.len();
        let cap = src.capacity();
        let ptr = src.as_mut_ptr();

        mem::forget(src);

        let original_capacity_repr = original_capacity_to_repr(cap);
        let arc = (original_capacity_repr << ORIGINAL_CAPACITY_OFFSET) | KIND_VEC;

        Inner {
            arc: AtomicPtr::new(arc as *mut Shared),
            ptr: ptr,
            len: len,
            cap: cap,
        }
    }

   #[inline]
    fn with_capacity(capacity: usize) -> Inner {
        if capacity <= INLINE_CAP {
            unsafe {
                // Using uninitialized memory is ~30% faster
                let mut inner: Inner = mem::uninitialized();
                inner.arc = AtomicPtr::new(KIND_INLINE as *mut Shared);
                inner
            }
        } else {
            Inner::from_vec(Vec::with_capacity(capacity))
        }
    }

}

impl BytesMut {
    #[inline]
    pub fn with_capacity(capacity: usize) -> BytesMut {
        BytesMut {
            inner: Inner::with_capacity(capacity),
        }
    }
    
    #[inline]
    pub fn capacity(&self) -> usize {
        self.inner.capacity()
    }
}

fn main() {
    let buf = BytesMut::with_capacity(1024);
    println!("{}", buf.capacity())
}

Running with Miri in the playground gives:

error[E0080]: constant evaluation error: Location is not frozen long enough
  --> src/main.rs:95:30
   |
95 |                 let p: &u8 = mem::transmute(arc);
   |                              ^^^^^^^^^^^^^^^^^^^ Location is not frozen long enough
   |
note: inside call to `Inner::kind::imp`
  --> src/main.rs:109:9
   |
109|         imp(&self.arc)
   |         ^^^^^^^^^^^^^^
note: inside call to `Inner::kind`
  --> src/main.rs:66:9
   |
66 |         self.kind() == KIND_INLINE
   |         ^^^^^^^^^^^
note: inside call to `Inner::is_inline`
  --> src/main.rs:119:12
   |
119|         if self.is_inline() {
   |            ^^^^^^^^^^^^^^^^
note: inside call to `Inner::capacity`
  --> src/main.rs:177:9
   |
177|         self.inner.capacity()
...

@RalfJung

[RalfJung]

Thanks! I am afraid this code most certainly has a data race, see this thread on IRLO. But that's not what miri complains about here.

This also transmutes an &UnsafeCell<T> to an &T, which is not okay before the latter expects the location to be frozen. So I think miri works as expected here, just bytes does not conform with Stacked Borrows.

A simple fix is to use *const u8 instead of &u8 in fn kind.

[RalfJung]

Fixed by tokio-rs/bytes#233.