Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
eddyb opened this issue Aug 24, 2021 · 2 comments
Open
Labels
A-layout Area: Memory layout of types C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Aug 24, 2021

I was looking back on #61956 (comment) 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:

  • T is a generic param (or an associated type of one)
    • presumably T: ?Sized, because T: Sized would result in Pointer(Inner(T)) having a known layout, even when the exact choice of T isn't known
  • Inner{1,2}(T) wrap T in any number of structs, with any choice of prefix fields
    • only the effect on Pointer(Inner(T)) containing T::Metadata matters
  • Outer{1,2}(X) are newtypes
    • that is, they only have an X field and optionally some ZST fields
    • we also allow Option-like enums here, if the Pointer type they wrap is non-null, but that shouldn't matter - it's effectively "desugaring" the enum optimization to a newtype
      • however, it does matter whether the enum opted out of the optimization, which turns out we're not checking

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
Copy link
Member

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
Copy link
Member

nagisa commented Aug 28, 2021

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.

@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 9, 2023
@Enselic Enselic added A-layout Area: Memory layout of types C-bug Category: This is a bug. labels Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-layout Area: Memory layout of types C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants