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.

---

Original Issue Description

#![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>(T);

struct Breakit {
    x: u8,
    y: Unalign<f32x4>
}

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:

    movups  49(%rsp), %xmm0
    movaps  %xmm0, (%rsp)
    #APP
    #NO_APP

But for the bad one, it is

    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.

[Gankro]

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.

[cmr]

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 rust-lang/rfcs#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 rust-lang/rfcs#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.

[Eljay]

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]

We discussed this in the @rust-lang/compiler meeting today. Nobody has written up any instructions =) but we did discuss some of the more subtle points:

Currently, we strip dead code out of MIR very early. This would mean that if we do 'unsafe' checking on MIR, we will ignore dead code (e.g., return; <this code>). We already ignore a number of errors in dead-code, and borrowck will totally ignore it, so this seems potentially just fine, but it's worth making a firm decision here.

There is another option: we can run the "unsafe check" before we strip dead code. The tricky part is that in that case some of the types in MIR may not match, since the type-checker ignores data-flow out of dead-code. This isn't really a problem for the unsafeck, but more generally it would mean the MIR is not "well-typed" and it'd be sort of nice to have an invariant that MIR is always well-typed.

One last point: we have to be careful that if false { <sort-of-dead-code> } doesn't get stripped out of MIR before we are done doing the various safety checks! It's one thing to suppress errors in code that is obviously dead, but quite another to led the results of constant propagation feed into safety analysis in that way.

[joshtriplett]

@nikomatsakis

One last point: we have to be careful that if false { } doesn't get stripped out of MIR before we are done doing the various safety checks! It's one thing to suppress errors in code that is obviously dead, but quite another to led the results of constant propagation feed into safety analysis in that way.

Especially because that conditional might look like if cfg!(...) or similar.

[RalfJung]

Actually, it's not even necessary to write & to get such unaligned loads -- having Drop type in a packed struct is also a problem, because drop gets an &mut and hence assumes it is aligned. (I noticed this while playing around with miri and suddenly getting unaligned access errors for boxes in packed structs.)

So, beyond making references to packed fields unsafe, it seems we also have to forbid types in packed structs that implement Drop. Will this need an RFC (despite being a soundness fix)?

[retep998]

Because references should always be aligned... how are we supposed to get a raw pointer to a given field to check whether it is in fact aligned or even do an unaligned read or write? Doing &foo as *const T doesn't really work because it goes through an intermediate reference.

[nikomatsakis]

@RalfJung huh, interesting. We could amend the existing RFC to cover that case -- i.e., open a PR amending the original text -- but I think it also suffices to open up a Rust issue, tag it as T-lang, and do the FCP there; I feel like it's in the spirit of the existing RFC, so I don't know that a full PR on the RFC repository is needed. (It'd be nice to clarify these procedures, this question comes up somewhat regularly; but the intention is that an RFC, once landed, can be amended in reasonable ways, and any concerns will be addressed and discussed in the tracking issue and -- ultimately -- during stabilization.)

[RalfJung]

Notes from IRC: https://botbot.me/mozilla/rustc/2017-07-31/?msg=89222091&page=2

[SimonSapin]

The next step is to port the unsafety check to operate on MIR, instead of HIR. I'd be happy to mentor such a thing, though it's probably complex enough that it makes sense to sync up online.

@nikomatsakis Is this still relevant? This sounds like a significant task of its own, should it be a separate issue?

[arielb1]

@SimonSapin

I am working on it right now.

[Zoxc]

For Drop fields, could we not move them into an aligned temporary before calling drop?

[arielb1]

@Zoxc

That wouldn't work for unsized fields (yes you can have repr(packed) + unsized), and it could create unexpected stack copies.

I think we should try to forbid repr(packed) structs with destructors and see how that goes.