UB + couple of mistakes in the string post

[GoldsteinE]

Hi! I enjoyed reading your latest post about yarn, so I decided to play around with it a little. There’re a couple problems with code as present in the post (I’m not sure about the code in the actual library).

The main problem is that this function:

pub fn owned(data: Box<str>) -> Self {
    let len = data.len();
    let ptr = data.as_ptr().cast_mut();
    mem::forget(data);

    Self {
      raw: RawYarn::from_raw_parts(HEAP, len, ptr),
      _ph: PhantomData,
    }
}

contains UB. Calling mem::forget(data); asserts unique ownership of data and invalidates ptr. That’s a common pitfall when using mem::forget(). The right way to do it is this:

pub fn owned(data: Box<str>) -> Self {
    let data = ManuallyDrop::new(data);
    let len = data.len();
    let ptr = data.as_ptr().cast_mut();

    Self {
        raw: RawYarn::from_raw_parts(HEAP, len, ptr),
        _ph: PhantomData,
    }
}

I’ve also noticed (and fixed) a couple of typos and minor mistakes that prevented the code from compiling. Here’s the playground, with all the relevant changes marked with (!) for easy search: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=bb572789e965be9ce194889684bf2afa. You can run it under Miri to check that UB is present in the first version and absent in the second.

I’d submit a PR, but I’m not sure where the sources for the posts are.

[mcy]

Thanks for providing those typo fixes. I'll apply them to the source for the article.

Miri thinks this 0 is "UB" for stupid reasons that boil down to materializing a &Box<str> as a subexpression. If you rewrite the code as follows Miri does not complain.

  pub fn owned(mut data: Box<str>) -> Self {
    let len = data.len();
    let ptr = data.as_mut_ptr();
    mem::forget(data);

    Self {
      raw: RawYarn::from_raw_parts(HEAP, len, ptr),
      _ph: PhantomData,
    }
  }

Incidentally, this is a problem unique to Box and the compiler's special knowledge of it (primarily a historical accident). Arguably, Miri is incorrect for flagging this, since it works fine for Vec:

fn main() {
    // Works fine (but triggers miri's leaksan, which is not UB).
    let mut data = vec![1];
    let ptr = data.as_ptr().cast_mut();
    std::mem::forget(data);
    unsafe { &mut *ptr; }
    
    // Spurious tagging error.
    let mut data = vec![1].into_boxed_slice();
    let ptr = data.as_ptr().cast_mut();
    std::mem::forget(data);
    unsafe { &mut *ptr; }   // Tagging error occurs here. 
}

This is the sort of weird gotcha that shouldn't be UB, particularly since it's not justified by corresponding potential compiler optimizations.

[Kolsky]

Thanks for providing those typo fixes. I'll apply them to the source for the article.

Miri thinks this 0 is "UB" for stupid reasons that boil down to materializing a &Box<str> as a subexpression. If you rewrite the code as follows Miri does not complain.

  pub fn owned(mut data: Box<str>) -> Self {
    let len = data.len();
    let ptr = data.as_mut_ptr();
    mem::forget(data);

    Self {
      raw: RawYarn::from_raw_parts(HEAP, len, ptr),
      _ph: PhantomData,
    }
  }

Incidentally, this is a problem unique to Box and the compiler's special knowledge of it (primarily a historical accident). Arguably, Miri is incorrect for flagging this, since it works fine for Vec:

fn main() {
    // Works fine (but triggers miri's leaksan, which is not UB).
    let mut data = vec![1];
    let ptr = data.as_ptr().cast_mut();
    std::mem::forget(data);
    unsafe { &mut *ptr; }
    
    // Spurious tagging error.
    let mut data = vec![1].into_boxed_slice();
    let ptr = data.as_ptr().cast_mut();
    std::mem::forget(data);
    unsafe { &mut *ptr; }   // Tagging error occurs here. 
}

This is the sort of weird gotcha that shouldn't be UB, particularly since it's not justified by corresponding potential compiler optimizations.

Stop lying please.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8f96ca9dd2d1df0bb4da73a55d355cab
Miri fails as soon as you try using this pointer. Of course nothing happens if you simply discard it.
The model doesn't work as you described. Box does have noalias attribute which means it behaves approximately like &mut T. Moving the unique reference invalidates all nested/child reborrows of said reference. As far as optimizations are concerned, you don't have data at hand to be sure if this is justified decision or not. As for Vec, the code that you wrote which supposedly works isn't technically greenlit by rust-lang devs – there were even discussions whether changing internals to a boxed slice should be done – but in the end too many crates intuitively relied on permitted aliasing implicitly and the decision was postponed. This was never the case with the Box.

See also: rust-lang/unsafe-code-guidelines#326

[GoldsteinE]

While I disagree with the tone of the previous message, it’s correct in its factual part: move of the Box<_> does invalidate pointers, and that’s not a bug in Miri.

[mcy]

What the hell are you talking about? Please take this toxic line of discussion elsewhere instead of trying to mansplain how LLVM works to me.