Added -Z randomize-layout flag

[Kixiron]

An implementation of #77316, it currently randomly shuffles the fields of repr(rust) types based on their DefPathHash
r? @eddyb

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

Please see the contribution instructions for more information.

[Kixiron]

@rustbot label +A-layout +T-compiler

[Lokathor]

So this makes structs bigger than the size of the sum of their fields?

Particularly, this makes structs with a single field bigger than that one field?

[chorman0773]

I believe that's the intention of the flag, yes. Yes, this would break code that is incorrectly relying on the layout of repr(Rust) types.

…

On Thu, Aug 26, 2021 at 09:09 Lokathor ***@***.***> wrote: So this makes structs bigger than the size of the sum of their fields? Particularly, this makes structs with a single field bigger than that one field? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#87868 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGLD262YA5AJJVWQ3YLL2LT6Y4IBANCNFSM5BYVL2PQ> .

[Lokathor]

On the one hand yes, on the other hand this flies against the current best estimation of the UCG.

I am aware it isn't RFC'd, but I'm not sure that part of field randomization is a good idea even if there's no RFC.

[the8472]

It could be limited to structs with more than 1 non-zst fields of different types. Of course that depends on what exactly the UCG mean by "homogenous structs", which is still an open question.
We could be conservative and look for fields of different size or alignment, but that wouldn't catch cases where people try to transmute between Vecs for example where the only internal difference is Unique<T> vs. Unique<U>.

[Lokathor]

Primarily I'm mostly concerned about single field structs. People should be able to, in general, trust their newtypes. Like even if you forget to specifically write down repr(transparent), rust has no legitimate reason to break the layout of a single field struct.

Secondarily I'm slightly concerned about situations with multi-field structs where this introduces padding bytes that were not present within the struct previously. Re-ordering fields so that the structure doesn't get bigger and have padding bytes added I'm all for, but if there is a Point(i32,i32) people should be able to look at that and assume no padding in the structure.

Though ultimately this is a debug sort of flag so it's not the end of the world if turning it on makes things go wild, it's better if turning on the flag doesn't trigger UB and make the entire debug attempt pointless because there's UB in the program.

[chorman0773]

On Thu, Aug 26, 2021 at 10:38 Lokathor ***@***.***> wrote: Primarily I'm mostly concerned about single field structs <https://rust-lang.github.io/unsafe-code-guidelines/layout/structs-and-tuples.html#single-field-structs>. People should be able to, in general, trust their newtypes. Like even if you forget to specifically write down repr(transparent), rust has no legitimate reason to break the layout of a single field struct.

Perhaps, perhaps not. What happens if you had a #[repr(Rust)] struct Foo(u8); on a platform that only has native 32-bit reads (and has to emulate smaller ones)? It would be more efficient to add 3 padding bytes to Foo, but if it becomes guaranteed that Foo is layed out like u8, now there is a potential layout optimization left on the table.

Secondarily I'm slightly concerned about situations with multi-field structs where this introduces padding bytes that were not present within the struct previously. Re-ordering fields so that the structure doesn't get bigger and have padding bytes added I'm all for, but if there is a Point(i32,i32) people should be able to look at that and assume no padding in the structure.

Though ultimately this is a debug sort of flag so it's not the end of the

world if turning it on makes things go wild, it's better if turning on the flag doesn't trigger UB and make the entire debug attempt pointless because there's UB in the program.

Well, at that point, transmute would error. You could also run the result through miri, where the UB will be trapped.

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <#87868 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGLD23KJRR7SBYCUHQ3EE3T6ZGVNANCNFSM5BYVL2PQ> .

[Lokathor]

Sure, transmute itself will error, but other forms of transmutation will not. For example, the most popular crate for safe transmutation abstraction (bytemuck) doesn't even use the transmute function at all, because that function doesn't work with generics.

[chorman0773]

bytemuck can't be used with repr(Rust) types, can it? Pod requires the type be repr(C) or repr(transparent) explicitly.

…

On Thu, Aug 26, 2021 at 10:57 Lokathor ***@***.***> wrote: Sure, transmute itself will error, but other forms of transmutation will not. For example, the most popular crate for safe transmutation abstraction (bytemuck) doesn't even use the transmute function at all, because that function doesn't work with generics. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#87868 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGLD24AFSVHTDZZTUQYA5LT6ZI4HANCNFSM5BYVL2PQ> .

[Lokathor]

Yes, that is what's documented in bytemuck, but people have misused it before despite the docs. Then they come to me and say "why did your library break my code?" and I have to look close at their program and point out what's wrong and so on.

Also, that's just one example to show that transmute itself isn't the only thing used to do transmutation.

My general point is that while some field randomization is probably fine we should not do too much of it and give trouble to people who are living in a gray zone.

[chorman0773]

Well, the idea behind the flag is to find code that relies on something it shouldn't. It's designed to be opt-in, and I'm sure that someone was transmuting things (not via mem::transmute), and used this flag in conjunction with miri, they'd get a very loud error.

[Kixiron]

@eddyb This is ready for review, compiler-team/#457 was accepted

[eddyb]

Under the assumption that the MCP (rust-lang/compiler-team#457) was enough:
@bors r+

[bors]

📌 Commit 09f1542 has been approved by eddyb