Closed
Description
As discussed on Reddit, it turns out that casting &mut T
to &mut MaybeUninit<T>
is not a safe operation, since it allows UB in safe code.
Playground example demonstrating the UB: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=93dd41cf851bfc0de2233e0a83b4b778
It is probably a good idea to explicitly point that out in MaybeUninit docs.
Activity
jannickj commentedon Nov 24, 2019
In the example you are already required to wrap the code in
unsafe
so I am curious why it's necessary to state explicitly forMaybeUninit<T>
? transmute is unsafe for most types not justMaybeUninit
.Kixunil commentedon Nov 24, 2019
@jannickj if you try to write a safe wrapper around
MaybeUninit<T>
, then the fact thatT
can be soundly transmuted intoMaybeUninit<T>
does not mean that you can transmute&mut T
into&mut MaybeUninit<T>
and expose the resulting reference to safe code. That's something easily overlooked.FWIW, there's at least one crate in the wild that is unsound because of this mistake. Hey @danielhenrymantilla you probably want to fix it. I wrote you an e-mail regarding other stuff, no reply yet.
danielhenrymantilla commentedon Nov 24, 2019
@Kixunil yes, thanks for the e-mail, I've been quite busy lately so I have not had much time to address this (or answer the e-mail 😉), but I'll try to have it fixed by tomorrow, with the previous versions being yanked (and the repo being published).
So, to summarize the issue, it turns out that
&'a mut MaybeUninit<T>
is far stronger than anOutRef<'a, T>
, since it allows writing garbage. If we had anOutRef<'a, T>
abstraction that only allowed to write aT
into the pointee, then downgrading a&'a mut T
to anOutRef<'a, T>
should be sound, right? And I expect&'a mut MaybeUninit<T> -> OutRef<'a, T>
to be sound as well.Which, when
mem::needs_drop::<T>().not()
, means thatOutRef<'_, T>
has no downsides to write dava w.r.t.&'_ mut T
Thanks @Shnatsel and @Kixunil for reporting this!
Kixunil commentedon Nov 25, 2019
Yes, I believe that's sound. Since I didn't get the answer to the e-mail I started creating a new crate from scratch that's addressing my needs. :) It's not published yet, but has bunch of other cool stuff. (Solves the same problem for slices, has tons of helper methods, iterator, safe cursor...) We can still cooperate if you want.
Kixunil commentedon Nov 25, 2019
FYI, here's the crate: https://crates.io/crates/possibly_uninit I hope it will help prevent these kinds of bugs.
BufMut
for&mut [u8]
is unsound tokio-rs/bytes#328Kixunil commentedon Nov 27, 2019
Interestingly
BytesMut
frombytes
crate is also unsound. I fear that stabilizingMaybeUninit
without having safe (or at least sound) abstractions for most common stuff was a major footgun.Kixunil commentedon Nov 27, 2019
How hard would it be to add some basic abstractions into
core
to avoid others screwing up? Does it require RFC? Maybe just something for slices and mutable references, not stabilizing too many methods yet.RalfJung commentedon Nov 28, 2019
To me this looks like someone mistakenly assumed
&mut T
to be covariant inT
: in some sense,MaybeUninit<T>
is a supertype ofT
, but that doesn't mean you can convert them below an&mut
.But, sure, if someone has a good suggestions for where in the
MaybeUninit
docs to put this, that seems fine.RalfJung commentedon Nov 28, 2019
@Kixunil Having a user-defined
OutRef
type seems like a reasonable approach. However, it looks like you are still using "uninit" terminology in your crate. I am particularly worried about this type. An "out ref" and a "reference toMybeUninit
" are two fundamentally different concepts as this issue shows, so please be careful in picking terminology that conveys that difference! Your current names, I am afraid, will only make things worse. A "MaybeUninitSlice" should be just&mut [MaybeUninit<T>]
, by using the same term for the totally different concept of anOutSlice
you might cause confusion.See #63569. However, what you are asking for is not "something for slices and mutable references to
MaybeUninit
"; what you are asking for is something entirely distinct, namely write-only references for initialization purposes.Kixunil commentedon Nov 28, 2019
Thanks for recommendation, good point, I will change it (that's why I called it "preview" :)). Is the name
OutRef
something standard and widely understood or just a suggestion? I didn't hear such thing before.RalfJung commentedon Nov 28, 2019
&out
as complement to&mut
has come up in discussions again and again over the years. This is related to in-place initialization / "placement new", which has a looooooong history in Rust. Swift has "in" and "inout" references; "out" seems like a natural name for this pattern.However, note that there is AFAIK no way to implement safe
&out
as a library. The issue is that you need to somehow force a write to happen. (Well there might be tricks you can play with generative lifetimes but it's going to not be very ergonomic.) TheBufMut
issue is special because the safety of the problematic API relies on things already being initialized to begin with. So, I think there are very few cases whereOutSlice
actually makes an API safe. That type is not truly an out-reference as it doesn't enforce initialization. Hm.Kixunil commentedon Nov 28, 2019
OK, I will rename it to
OutRef
andOutSlice
.Yes, the lack of enforcing is something I noticed. It's the primary reason I added
Cursor
for slices and I was thinking about doing something similar forSized
types (call itSlot
? IDK).It is totally true that "
OutSlice
" is not very useful in itself as the code interacting with it will be mostlyunsafe
(it can be nicely seen inCursor
impls). However, I do think that preventing footguns inunsafe
code still has a value. At least documentation value that could prevent mistakes like these.6 remaining items
MaybeUninitSlice::from_init_mut
and useless `...::fr… WaffleLapkin/arraylib#21danielhenrymantilla commentedon Oct 15, 2020
By the way, this issue is related to another interesting one: for
&T -> &MaybeUninit<T>
to be sound, we need to express thatUnsafeCell
andMaybeUninit
do not commute! More info in the docs of::uninit
: https://docs.rs/uninit/0.4.0/uninit/out_ref/struct.Out.html#interior-mutabilitytesuji commentedon Oct 17, 2020
When I first saw the title, I was expecting that casting is
&mut T as &mut MaybeUninit<T>
.Isn't it a transmute and not a cast ? I would like to change the issue title to reflect that.
Kixunil commentedon Oct 17, 2020
Is
&mut *(foo as *mut _ as *mut MaybeUninit<T>)
considered transmute?Anyway, the title should (also)
s/safe/sound/
. The fact it'sunsafe
is already obvious.thomcc commentedon Oct 30, 2020
It's not always unsound, just sometimes. The usual way of describing this is unsafe.
ojeda commentedon Jan 19, 2021
Something cannot be sometimes unsound -- it is either unsound or not. Unsafe does not imply unsound or sound.
thomcc commentedon Jan 19, 2021
It's unsound if you write MaybeUninit::uninit to it, or expose it to safe code you don't control (that can do this trivially). It's sound otherwise.
ojeda commentedon Jan 19, 2021
That means it is unsound. It doesn't matter that sometimes there may not be code triggering the UB.
Kixunil commentedon Jan 19, 2021
So the title should be "Document that casting &mut T to &mut MaybeUninit and exposing it to safe code is not sound"
transmute<&mut T, &mut MaybeUninit<T>>
is unsound when exposed to safe code #134583Rollup merge of rust-lang#134583 - Enselic:maybe-uninit-transmute, r=…
Unrolled build for rust-lang#134583
Rollup merge of rust-lang#134583 - Enselic:maybe-uninit-transmute, r=…