rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.6k stars 12.62k forks source link

Transmute special-case doesn't take into consideration alignment or enum repr. #88290

Open eddyb opened 3 years ago

eddyb commented 3 years ago

I was looking back on https://github.com/rust-lang/rust/issues/61956#issuecomment-505436119 and since the current implementation hasn't changed since, I decided to try to and see if I can create an example.

And yes, here is a playground example showing three invalid transmutes, all stemming from different points in the code (two alignment-based and one regarding treating "non-null-optimized" enums as newtypes).


What we special-case is a transmute from Outer1(Pointer1(Inner1(T))) to Outer2(Pointer2(Inner2(T))), where:

Now, I confused myself with alignment originally, but what's important to note is that even if a newtype of X contains higher alignment ZSTs around X, the only difference that will make is on the alignment (and size, through additional padding) of the whole newtype, not on the position of X in the newtype.

Because of that, Outer1 and Outer2 could have different alignments and that wouldn't change the fact that reading the pointer from one of them and writing it into the other would work, it's still at offset 0 and has the same size.

But that's not what we do, since transmute is not field-based, and if we copy the larger size, we're reading or writing more bytes than would be legal.


Looking at the LLVM IR of my example, test_align does look like it's copying 64 bytes, which should definitely be UB (but it might take some effort to cause a LLVM optimization to trigger).

OTOH, test_option_like is even worse, since the pointer goes in the #[repr(C)] enum tag, and the value inside the Some is garbage, so running it in release mode crashes trying to print the resulting value.


What's a bit sad is I feel like I remember seeing an assert! for equal transmute sizes in codegen, which would catch such a situation and turn it into an ICE, but I'm guessing it got removed?

cc @nagisa

RalfJung commented 3 years ago

What's a bit sad is I feel like I remember seeing an assert! for equal transmute sizes in codegen, which would catch such a situation and turn it into an ICE, but I'm guessing it got removed?

We have such an assert in Miri, and indeed your code ICEs in Miri. But I am not sure about codegen.

nagisa commented 3 years ago

What's a bit sad is I feel like I remember seeing an assert! for equal transmute sizes in codegen, which would catch such a situation and turn it into an ICE, but I'm guessing it got removed?

We used to emit some of the size mismatch errors from the old trans and were using LLVM sizes to verify validity of the transmute. This most likely was lost in migration to MIR based codegen, as I don't recall anything of the sort being present in the MIR codegen.