repr(packed) allows invalid unaligned loads

[huonw]

This is now a tracking issue for RFC 1240, "Taking a reference into a struct marked repr(packed) should become unsafe", but originally it was a bug report that led to the development of that RFC. The original miscompilation did not involve references, but it has been fixed; the reference case is the one that remains open. A lot of the discussion is hence outdated; this is the current state.

---

Original Issue Description

(This code actually works [fine on today's nightly](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=baa653df7e8938f20c7f5448ffef64c1).) ``` rust #![feature(simd, test)] extern crate test; // simd types require high alignment or the CPU faults #[simd] #[derive(Debug, Copy, Clone)] struct f32x4(f32, f32, f32, f32); #[repr(packed)] #[derive(Copy, Clone)] struct Unalign(T); struct Breakit { x: u8, y: Unalign } fn main() { let val = Breakit { x: 0, y: Unalign(f32x4(0.0, 0.0, 0.0, 0.0)) }; test::black_box(&val); println!("before"); let ok = val.y; test::black_box(ok.0); println!("middle"); let bad = val.y.0; test::black_box(bad); println!("after"); } ``` Will print, on playpen: ``` before middle playpen: application terminated abnormally with signal 4 (Illegal instruction) ``` The assembly for the `ok` load is: ``` asm movups 49(%rsp), %xmm0 movaps %xmm0, (%rsp) #APP #NO_APP ``` But for the `bad` one, it is ``` asm movaps 49(%rsp), %xmm0 movaps %xmm0, (%rsp) #APP #NO_APP ``` Specifically, the `movups` (unaligned) became a `movaps` (aligned), but the pointer isn't actually aligned, hence the CPU faults.

[Gankra]

I... honestly thought that was expected behaviour. What can possibly be done about this is general, other than making references into packed fields a different type (or forbidding them)?

[eefriedman]

This is UB, so it's clearly not expected... but yes. it's more of a design flaw than an implementation issue.

Note that it's possible to make code like this misbehave even without SIMD, although it's a bit trickier; LLVM performs optimizations based on the alignment of loads.

[huonw]

Yeah, I only used SIMD because it was the simplest way to demonstrate the problem on x86. I believe platforms like ARM are generally stricter about load alignments, so even, say, u16 may crash in simple cases like the above.

[retep998]

Is there a way to tell LLVM that the value could be unaligned, and so LLVM should emit code that tries to read it in a safe way for the given architecture, even if it results in slower code?

[vadimcn]

@retep998: That's exactly what LLVM should have done for a packed struct. This looks like a LLVM codegen bug to me.

[dotdash]

@vadimcn this is entirely our (or rather: my) fault. We used to not emit alignment data at all, which caused misaligned accesses for small aggregates (see #23431). But now we unconditionally emit alignment data purely based on the type that we're loading, ignoring where that value is stored. In fact at the point where we create the load, we currently don't even know where that pointer comes from and can't properly handle packed structs.

[retep998]

Perhaps we need some sort of alignment attribute that we can attach to pointers?

[huonw]

In general we have no idea where a reference comes from, e.g. fn foo(x: &f32x4) { let y = *x; ... in some external crate can't know if we happen to pass in &val.y.0 in the code above. The only way to codegen to handle unaligned references properly is to assume pointers are never aligned, which would be extremely unfortunate. We don't currently have type-attributes, so it would be somewhat strange to introduce one here, instead of using types (for example).

[vadimcn]

Can we make creation of references into packed structs unsafe?

[nikomatsakis]

triage: P-high

[nikomatsakis]

Seems clear there's a bug here. First step is to evaluate how widely used packed is, so huon is going to do a crater run with #[repr(packed)] feature gated.

[huonw]

I made a patch that feature gated repr(packed) and did a crater run. Results: https://gist.github.com/huonw/8262a4fb2da0a052f5f1 . Summary:

High-level summary:

• several unnecessary uses of repr(packed)
• most necessary ones are to match the declaration of a struct in C
• many "necessary" uses can be replaced by byte arrays/arrays of smaller types
• 8 crates are currently on stable themselves (unsure about deps), 4 are already on nightly
  • 1 of the 8, http2parse, is essentially only used by a nightly-only crate (tendril)
  • 4 of the stable and 1 of the nightly crates don't need repr(packed) at all

	stable	needed	FFI only
image	✓
nix	✓	✓	✓
tendril		✓
assimp-sys	✓	✓	✓
stemmer	✓
x86	✓	✓	✓
http2parse	✓	✓
nl80211rs	✓	✓	✓
openal	✓
elfloader		✓	✓
x11	✓
kiss3d	✓

As you can see from the "huon references this issue ..." spam above this comment, I've opened PRs against the libraries where the repr(packed) seems to be unnecessary.

[emberian]

The bug here is the testcase, it's intentionally violating CPU alignment constraints. In C, packed is unsafe, and you're not supposed to take pointers to the fields of a packed structure, for fear of unaligned access. So, a simple path forward to me seems to be disallow taking references to fields of a struct with #[repr(packed)].

[eddyb]

I like @cmr's suggestion, on which I see 3 variations:

• hard error on all borrows (most conservative)
• copy an rvalue out and borrow that (likely to avoid errors in existing code)
• make borrows unsafe

The first two have the advantage that we could pack things tighter, e.g. bool fields could be bit-fields. Not sure that's something we want to do to #[repr(packed)], but nevertheless, opting into no references to fields will be needed at some point for more layout optimizations.

@huonw Could those crates be checked with an error being emitted here if cmt refers to a field of a #[repr(packed)] struct, or something inside such a field?

[glaebhoerl]

cc https://github.com/rust-lang/rfcs/issues/311 (and also #[repr(simd)], all of which are in the same situation)

[glaebhoerl]

brief sketch of how all of these could work:

• for interior & refs, copy out onto stack first
• for interior &mut, copy out first, and copy back immediately after borrow ends (subtlety: if ownership of &mut is passed to another fn, copy-back is still done by the caller (because callee can't know whether it should); difference should not be observable because &mut implies no other references exist through which to observe; same logic suggests mem::forget shouldn't be a problem either)
• wrinkle: interior mutability aka UnsafeCell, needs to either fall back to normal repr, compile error ("can't #repr(foo) on types containing UnsafeCell..."; wrinkle: what about generic types?), or some other special handling
• question: are "UnsafeCell" and "not UnsafeCell" the only two possible relevant cases?

(sorry for brevity, been wanting to write this down for a while but short of time)

[eddyb]

@glaebhoerl That's an interesting suggestion, and I can't think of any issues with the &mut handling. So for a struct with no interior mutability, would the only difference with #[repr(packed)] be the lifetime of field borrows, i.e. &'a Struct -> &'a Field wouldn't work?

[glaebhoerl]

Oh, hmm. To be honest I hadn't even thought of that. (In case it's not clear to anyone else (it only became clear to me after I had written out a comment asking for clarification): if I have a PackedStruct on my stack, and wish to call an fn get_field(&PackedStruct) -> &Field on it, that plainly won't work if get_field copies the Field out onto its own stack, which'll be deallocated by the time it returns.) Too bad: I had been hoping all #[repr(s)] could keep the same observable semantics, and even a subtlety like this rules out that possibility. Apparently there's no free lunch.

[eddyb]

@glaebhoerl I do expect that such a scheme would allow most code to continue compiling, given the small number of repr(packed) uses.

@huonw have you grepped for repr(packed) in the cargo dir to find occurrences in code that does not otherwise compile? (Just making sure nothing is missed, I don't foresee anything significant showing up.)

[retep998]

I use and need repr(packed) for one of my libraries, because I do memory mapping tricks. I'm fine with it being unsafe, just as long as I can continue to use it from within stable Rust.

[huonw]

The bug here is the testcase, it's intentionally violating CPU alignment constraints. In C, packed is unsafe, and you're not supposed to take pointers to the fields of a packed structure, for fear of unaligned access. So, a simple path forward to me seems to be disallow taking references to fields of a struct with #[repr(packed)].

Of course the bug is in the test case... that's why I filed an issue with it ;) It's a compiler bug that the test case compiles and then crashes: it should either not compile, or not crash. It's triggering undefined behaviour (at the very least, we're telling LLVM that the load is aligned when it isn't), and, notably, it's particularly egregious, since there's no explicit internal references at all: our current (broken) scheme could probably be tweaked to make that specific example compile, without making any other changes.

(Of course, the fundamental problem won't be fixed by making a few tweaks to how we codegen struct field accesses.)

I use and need repr(packed) for one of my libraries, because I do memory mapping tricks. I'm fine with it being unsafe, just as long as I can continue to use it from within stable Rust.

Could you go into more detail about what the struct looks like?

[huonw]

have you grepped for repr(packed) in the cargo dir to find occurrences in code that does not otherwise compile?

Nothing new.

[retep998]

@huonw https://github.com/NoLifeDev/nx-rs/blob/master/src/repr.rs

[huonw]

It looks like that might be able to get away with [u32; 2] + some transmutes in a pinch (highly not great of course...).

(I assume you've thought about the endianness issues that comes with mmaping data directly?)

[arcnmx]

Going to throw in my voice in that #[repr(packed)] is rather important for http://arcnmx.github.io/nue/pod/ (has a safe/stable interface via syntex). However, yes, they will get aligned wrong on ARMv5 (an arch that rust doesn't support), but will work on ARMv6 (rust's current minimum supported arm platform). Loading from a pointer with incorrect alignment is considered UB in LLVM though, so... ouch. Just because it works in most cases on x86 doesn't mean it can be ignored :(

Note that borrows are only UB/unsafe for types that have an alignment > 1 - I would hope to retain this behaviour at the very least, so that my Le, u8, and other such types can continue to function properly in stable and safe code.

My proposal: If packed can be exposed as a marker trait, OIBIT could be used to ensure that packed types only contain other packed types (and thus all have alignments of 1, and borrowing any of its members will be safe). Then you can opt in to packed via an unsafe impl if you really want to use unaligned accesses, or don't care for whatever reason.

[comex]

Not that it really matters due to the LLVM UB issue, but it's still possible to disable unaligned accesses on ARM with a CPU flag; GCC and Clang expose -mno-unaligned-access to generate code that supports this.

[bluss]

Can this cause memory unsafety?

If it always leads to an exception and abort, maybe it is something that Rust can allow.

[arcnmx]

@bluss it can. Look at the linked issue about unaligned reads on ARM, it'll just silently give you wrong/unexpected values when reading from a pointer. It's also explicitly stated as undefined behaviour in LLVM:

It is the responsibility of the code emitter to ensure that the alignment information is correct. Overestimating the alignment results in undefined behavior. Underestimating the alignment may produce less efficient code. An alignment of 1 is always safe. The maximum possible alignment is 1 << 29.

[arcnmx]

Well, I put an initial implementation of this into http://arcnmx.github.io/nue/packed/index.html

#[packed] ensures that all types are safe to use unaligned before applying #[repr(packed)], and includes some helpers along with pod's usual endian primitives. The API needs some work, and some of my assumptions may not be entirely correct, but it gives a rough idea of how something similar could work for std!

[huonw]

I've written https://github.com/rust-lang/rfcs/pull/1240 which proposes making taking a reference into a repr(packed) struct unsafe.

[arcnmx]

I don't feel like this is the correct approach to take, will write up some notes in the RFC comments in a bit.

[retep998]

Also, I'd like to point out that, as it turns out, a bunch of types in the Windows SDK are packed, so #[repr(packed)] really needs to stay in stable Rust for winapi's sake.

[ghost]

Also, I'd like to point out that, as it turns out, a bunch of types in the Windows SDK are packed, so #[repr(packed)] really needs to stay in stable Rust for winapi's sake.

Might also be worth noting that some Windows API structs are packed to different alignments, e.g. #pragma pack(2), whereas Rust seems to currently only allow 1-byte alignment (although this can be easily worked around with additional padding fields).

[retep998]

Could we perhaps expand the syntax to #[repr(packed(N))] then?

[nikomatsakis]

Note that rust-lang/rfcs#1240 was accepted. I have re-used this as the tracking issue.

[nikomatsakis]

@rust-lang/compiler it'd be great to find someone to implement this! :)

[Aatch]

I'll take a look at it this weekend, I won't assign the issue to myself until I actually start working on it, just in case I decide that I don't have time (or forget!)

[retep998]

A new align metadata was added to the load instruction in LLVM.

http://reviews.llvm.org/rL248721

[huonw]

That doesn't solve the problem, unless we mark all loads out of a &T as having alignment 1 (i.e. the current set-up means we could never assume a &T is correctly aligned), which would be very unfortunate from a performance point of view.

[huonw]

Hey @Aatch, have you got an update the situation?

[arielb1]

@huonw This is basically waiting on MIR passes.

[nrc]

triage: P-medium

also waiting for mir

[Gankra]

Came up today that repr(packed) may also introduce data races. For instance, accessing a misaligned AtomicPtr, or the fields next to it, may do strange things. I'm unsure if this can be triggered in any way that isn't just "misaligned load". I don't know a lot about the relevant instruction sets and things LLVM does.

[retep998]

I'd be fine with not having atomics in repr(packed)

[nrc]

Given that this has been sat around for a while and that it sounds like it is not going to get implemented that soon, I think we should feature-gate #[repr(packed)] in the meantime. I propose we start with a warning cycle and uplift that to beta then put a feature gate in nightly now. Alternatively, we could just warn on use of #[repr(packed)] and never move to a feature gate (since it is widely used in WinAPI).

I think we should do something. Having potential memory unsafety sat around without the compiler telling the user about it at all is pretty bad.

[retep998]

Meanwhile in winapi #[repr(packed)] is the only tool I have to lower the alignment of structs to make their layout match that of the C version which uses #pragma pack. Please don't feature gate it without providing an alternative. Many features that I'd like for FFI, such as unions, I can manage without by using my own methods and macros. However without some way to place a 4 byte field on a 2 byte boundary, I'm simply stuck and can't represent those types.

[nikomatsakis]

I don't know, packed is used widely enough, and the vast majority of uses are fine. I expect the fix will come in a month or two, once the MIR transition has made more progress, but perhaps that is wildly optimistic.

On Sun, Feb 07, 2016 at 01:48:31PM -0800, Nick Cameron wrote:

Given that this has been sat around for a while and that it sounds like it is not going to get implemented that soon, I think we should feature-gate #[repr(packed)] in the meantime. I propose we start with a warning cycle and uplift that to beta then put a feature gate in nightly now. Alternatively, we could just warn on use of #[repr(packed)] and never move to a feature gate (since it is widely used in WinAPI).

I think we should do something. Having potential memory unsafety sat around without the compiler telling the user about it at all is pretty bad.

---

Reply to this email directly or view it on GitHub: https://github.com/rust-lang/rust/issues/27060#issuecomment-181126967

[nikomatsakis]

After some discussion in compiler team meeting:

• We can easily feature-gate references into fields of packed structs specifically.
• But MIR may be mature enough to just port over the unsafety checking and do this The Right Way. I a thinking of taking a stab at that because it sounds fun and I want to do some more MIR work. But if others really want to and I fail to make progress please ping me. :)

[bestouff]

Maybe I didn't understand everything, but it seems to me there are 2 different problems here: letting rustc squeeze every bit of space possible (e.g. bool on 1 bit, optimized enums) to have compact data structures, and choosing each struct member's footprint to match an FFI object layout (e.g. including a C bitfield with arbitrary length) - which can't always be done automatically. So repr(packet) is not an universal solution for data exchange with foreign libs.

[DemiMarie]

On x86-64, is it always possible to just use unaligned instructions? GHC had to edit the output of LLVM to deal with alignment-requiring AVX instructions.