Implementation of BufMut for &mut [u8] is unsound

[Kixunil]

Example how safe code can cause UB using BufMut:

let mut buf = [42];

BufMut::bytes_mut(&mut buf)[0] = MaybeUninit::uninit();

println!("Reading this value is UB: {}", buf[0]);

As discussed at rust-lang/rust#66699 casting &mut [T] to &mut MaybeUninit<T> isn't safe. Thus bytes_mut must return a newtype that prevents overwriting value with invalid values. See how I addressed it in my crate possibly_uninit.

It'd be the best to share common newtype slice implementation to not fragment the ecosystem. You can expect full support from me when it comes to using my crate as a dependency. That includes making you maintainers, if you're interested.

[seanmonstar]

I'm amazed and yet not surprised. Thanks for pointing this out! It does seem like a tragedy that MaybeUninit::uninit() is safe to call.

We may end up needing newtype in bytes as you mentioned. I need to think about this more.

[carllerche]

As sean pointed out, it seems that the source of UB is due to MaybeUninit::uninit() being safe to call. The fix would be for bytes to define its own UninitBuf type that only exposes *mut u8 and usize (len).

[carllerche]

While we should obviously fix this, I don't think it merits an emergency patch. We probably can wait until 0.6 (in a few months). This doesn't seem worse than the state of 0.4.

[Kixunil]

Yes, if MaybeUninit::uninit() was unsafe to call, then it wouldn't cause unsoundness. Since MaybeUninit was design to allow working with uninitialized memory safely, I think it's correct to make uninit() safe and all other code should ensure correctness. That being said, I suspect that not providing these abstractions in core when stabilizing MaybeUninit was a footgun.

What do you think about my approach of creating special slice type? Would just using it and depending on possibly_uninit be sufficient? Would you like some changes to it? Or would you like to avoid dependency?

I agree it's probably not emergency situation, maybe it should be documented, though.

[carllerche]

Something like your slice is probably better. I would rather avoid adding a dependency on a third party crate as we are pushing hard for 1.0.

[Kixunil]

Hmm, I see the good reason to avoid it because of pushing for 1.0. Would separating only the type itself into a separate crate help? Such crate could stabilize very quickly (Instantly?) and my crate would just depend on it too. Alternatively, it could be implemented in bytes directly, but I don't think it'd be nice to require people who don't want to do anything with bytes to depend on bytes just to get MaybeUninitSlice.

I think that this separate crate should be called core. Eventually. :)

[seanmonstar]

cc @RalfJung

[RalfJung]

Good catch!

It does seem like a tragedy that MaybeUninit::uninit() is safe to call.

Strongly disagreed. MaybeUninit was designed for a particular purpose, and for that purpose it makes no sense to make uninit() unsafe.

This looks to me like someone made the mistake of realizing that MaybeUninit<T> is like a supertype of T, and thus &mut MaybeUninit<T> also would be a supertype of &mut T -- but &mut T is invariant, not covariant, so that is not correct. Mistakes happen, and that's okay, but I wouldn't blame MaybeUninit's API for this.

That being said, I suspect that not providing these abstractions in core when stabilizing MaybeUninit was a footgun.

MaybeUninit was never intended to be used for what it is being used as here, and that use-case was also not even mentioned during the many, many months of pre-stabilization discussion.

[Kixunil]

Fair, do you think the apparent popularity justifies including some standardized types into core to prevent future accidents? It could be very conservative for starters and expanded later.

[RalfJung]

the apparent popularity

I don't see one instance of this bug as an indication of popularity. ;) An API that both is geared towards in-place initialization (and thus handles references to MaybeUninit<T>) and allows safe construction for borrowed already initialized data is a somewhat odd thing.

One could just as well argue that whatever safely turns an &mut [T] into a BufMut is wrong. I think I understand the design decisions that lead here, but I don't see any reason to expect this to happen frequently.

---

In fact, I think just making MaybeUninit::uninit() unsafe does not fix the soundness hole (assuming that there is a way to construct a BufMut for uninitialized memory, which is the entire point):

fn with_uninit_bufmut(buf1: impl BufMut) {
  let mut buf2 = [42];
  BufMut::bytes_mut(&mut buf2)[0] = buf1.bytes_mut()[0];
  // If `buf1` is uninitialized, now so is `buf2`.
}

[Kixunil]

I meant popularity of this crate and possibly pattern. But that may still be overstatement.

I think the whole thing is stemming from the fact that we didn't have a nice way of working with uninitialized memory from the beginning, so people were using &mut [u8] instead (see std::io::Read which unfortunately is a stable API), so now people are trying to make stuff backwards compatible.

If the primitives for working with uninit memory were in Rust from the beginning, I don't think we would be having this discussion.

Good point about uninit() being safe.

[RalfJung]

Good point about uninit() being safe.

Given that, is there a proposed fixed API? My counterexample will work for any &mut [Wrap<T>] where Wrap<u8> is Copy. It's not enough to have a slice-of-newtype, you really need a newtyped slice that somehow disallows this. Is there a concrete proposal for that?

[Kixunil]

Well, I proposed to cut out MaybeUninitSlice (obviously, renaming it as you recommended) from my crate into a separate one, which can be stabilized quickly, then return that thing instead of &mut [MaybeUninit<u8>]. I don't know if there's any better way.

[RalfJung]

Ah I see. Yes that API looks good on a cursory glance.

[carllerche]

I would say either something should exist in std or bytes will define something like MaybeUninitSlice specifically for [u8] (maybe UninitBuf or something). If there is going to be a standard that all crates use, it should be in std.

[Kixunil]

You mean core, right? There's no reason it needs OS or allocator. :)

[carllerche]

Sure. I use std / core interchangeably to mean "what rustc ships with" :)

[benma]

Imho this bug warrants a quick documentation update if a fix is taking longer. I was about to use the &mut [u8] implementation of BufMut and almost missed that it's not safe to use.

[carllerche]

Fixed by #433