Rust: Tracking issue for `safe_packed_borrows` compatibility lint

[arielb1]

What is this warning about

Fields of structs with the #[repr(packed)] attribute might be unaligned. Even if a field has a valid offset (e.g. 0) within the struct, the struct itself might be unaligned.

On the other hand, safe references (&T and &mut T) must always have valid alignment (even when they are just created) - not only will dereferencing these references cause segfaults on architectures such as Sparc, but the compiler is allowed to optimize based on that assumption (e.g. in a future version of rustc, it might use the "free" alignment bits to represent enum discriminants), so even just creating an unaligned reference is Undefined Behavior and might cause the program to behave in unexpected ways.

Therefore, borrowing a field in the interior of a packed structure with alignment other than 1 is unsafe - if the reference is not known to be aligned, the borrow must be done to an unsafe pointer.

Note that borrowing a field with alignment 1 is always safe.

For example, consider the common struct Unaligned:

#[repr(packed)]
pub struct Unaligned<T>(pub T);

That struct can be placed inside a bigger struct at an unaligned offset:

pub struct Foo {
    start: u8,
    data: Unaligned<u32>,
}

In that case, a borrow of a field of the struct would be Undefined Behavior (UB), and therefore is made unsafe:

let x = Foo { start: 0, data: Unaligned(1) };
let y = &x.data.0; // UB, also future-compatibility warning
println!("{}", x.data.0); // this borrows `x.data.0`, so UB + future-compat warning
use(y);

This used to be left unchecked by the compiler - issue #27060.

How to fix this warning/error

The most common ways to fix this warnings are:

1. remove the #[repr(packed)] attribute if it is not actually needed. A few crates had unnecessary #[repr(packed)] annotations - for example, [tendril].
2. copy the fields to a local first. When accessing a field of a packed struct directly without using a borrow, the compiler will make sure the access is done correctly even when the field is unaligned. The then-aligned local can then be freely used. For example:
   ``Rust let x = Foo { start: 0, data: Unaligned(1) }; let temp = x.data.0; println!("{}", temp); // works // or, to the same effect, using an{x}` block:
   println!("{}", {x.data.0}); // works
   use(y);
   ````

In some cases, it might be necessary to borrow the fields as raw pointers and use read_unaligned/write_unaligned to access them, but I haven't encountered such a case.

Derives

One annoying case where this problem can appear is if a packed struct has builtin derives, e.g.

#[derive(PartialEq, ...)]
#[repr(packed)]
pub struct Unaligned<T>(pub T);

Which essentially expands to this:

impl<T: PartialEq> PartialEq for Unaligned<T> {
    fn eq(&self, other: &Self) -> bool {
        PartialEq::eq(&self.0, &other.0) // this is UB and unsafe
    }
}

If your struct already derives Copy and has no generics, the compiler will generate code that copies the fields to locals and will therefore work. Otherwise, you'll have to write the derive implementations manually.

For example, you might want to add a T: Copy bound and copy things out:

#[repr(packed)]
pub struct Unaligned<T: Copy>(pub T);

impl<T: Copy + PartialEq> PartialEq for Unaligned<T> {
    fn eq(&self, other: &Self) -> bool {
        ({self.0}) == ({other.0}) // this copies fields to temps, and works
    }
}

[luser]

My rust-minidump crate has a bunch of packed structs derived from a C header that I ran bindgen on. Now that I've updated to a newer rustc I'm getting a slew of E0133 warnings. The fact that you get a warning using a field from a packed struct in println! is pretty annoying, especially when the field itself is Copy! Is there any way we can make this case more ergonomic?

[gnzlbg]

What's the time-line for turning this warning a hard-error on nightly, and moving towards making it a hard error on stable ? It has been a warning on nightly for a couple of releases already.

[RalfJung]

@luser
The println! likely actually takes a reference and uses it, so adding an unsafe block doesn't even fix anything -- it just makes your code compile with UB. Try wrapping the arguments in curly braces:

println!("{}", {self.packed_field});

This should work if they are Copy, should not require an unsafe block because no reference is taken, and properly avoids the UB.

---

However, I am worried many people do not realize what this unsafety really is about, and just add an unsafe block to silence the warning. I just read over the commit by @yodaldevoid that is linked above because they mentioned this issue (https://github.com/yodaldevoid/mkl26/commit/c1c1d5c6c946146d479a8023a4ccd3a7ed884f41), and it contains code like (comment is mine)

impl<'a> Adc<'a> {
    pub fn is_conv_done(&mut self) -> bool {
        // self.reg has type AdcRegs which is packed; then we call RW::read(&self, u32)
        unsafe { self.reg.sc1a.read().get_bit(7) }
  }
}

So, this code is UB if these fields are actually unaligned. @yodaldevoid, it would be interesting to learn why you thought that passing an unaligned reference to RW::read is okay, or did you ensure that the reference is actually aligned despite packing? (Looking at AdcRegs, it consists entirely of u32 and i32, so repr(packed) is likely not even needed?)

[gnzlbg]

@RalfJung the current warning message is pretty bad:

See it live (playground):

#[repr(packed)]
struct A {
    x: u8,
    y: u64
}

fn main() {
    let a = A { x: 1, y: 2 };
    let _ = &a.y;
}

outputs:

warning: borrow of packed field requires unsafe function or block (error E0133)
 --> src/main.rs:9:13
  |
9 |     let _ = &a.y;
  |             ^^^^
  |
  = note: #[warn(safe_packed_borrows)] on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

It says that taking a reference requires an unsafe block, which encourages users to do just that. Open an unsafe block and be done with it. It should at least mention why this is unsafe, and encourage users to fix these issues with care:

warning: borrow of packed field is unsafe (error E0133)
 --> src/main.rs:9:13
  |
9 |     let _ = &a.y;
  |             ^^^^
  |
  = note: #[warn(safe_packed_borrows)] on by default
  = warning: fields of packed structs might be misaligned: dereferencing a misaligned reference is undefined behavior. 
  = note: this is warning will turn into a hard error in the next Rust 1.2x.0 release. Taking references to packed struct fields allows undefined behavior to happen in safe Rust. Fix this with great care - for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

It is also "only" a warning, which encourage people that numbly pursue clean builds to just want to "silence it" without digging deeper. Also some users might conclude that, if this was important, it would be a hard error, saying that it will become an error at some undetermined point in the future doesn't really convey much seriousness.

[RalfJung]

@gnzlbg

the current warning message is pretty bad:

Yeah that probably doesn't help. However, I also feel this violates the usual pattern in Rust that you should not make unsafe operations accidentally. The only other non-function-call operations we have that are unsafe involve dereferencing a raw pointer, and everyone kind-of knows why that is unsafe (though they may forget about alignment). That the mere act of taking a reference can be UB will probably be surprising to many. If they are doing this e.g. in an unsafe fn, they will never even see the warning/error, no matter how much we improve it.

I think long-term we want to have an operation (that @arielb1 has proposed several times I think) that directly creates a raw pointer to a field, and we want to make taking a reference to a packed field outright illegal. People should use the raw pointer operations instead. However, I don't even know how the syntax for that should look like.

[gnzlbg]

I think long-term we want to have an operation (that @arielb1 has proposed several times I think) that directly creates a raw pointer to a field, and we want to make taking a reference to a packed field outright illegal.

Honestly, I would be fine with making taking references to fields of packed structs illegal at first. For FFI this is not something that's really needed. I don't know how that would interact with derives but at worst they just won't compile and users that want to can manually implement these.

However, I also feel this violates the usual pattern in Rust that you should not make unsafe operations accidentally.

Sure and I agree. As you mention, this is surprising and people can slip which is why we need to be more proactive here.

[RalfJung]

Honestly, I would be fine with making taking references to fields of packed structs illegal at first.

Given that this has been possible on stable for several years, I don't think that's realistic.

Though maybe we can make it illegal unless it is immediately followed by as *const _ or as *mut _?

[gnzlbg]

Though maybe we can make it illegal unless it is immediately followed by as *const _ or as *mut _?

That would be an improvement.

Given that this has been possible on stable for several years, I don't think that's realistic.

Turning this warning into an error is a breaking change, and breaking changes that fix soundness bugs are "allowed". What complicates this case is that the code might actually be working fine in the platforms that the users are actually using, and that without an escape hatch users don't have a "trivally-easy" way to work around this.

[RalfJung]

What complicates this case is that the code might actually be working fine in the platforms that the users are actually using,

We provide alignment annotations to LLVM irrespective of the platform. So, even on platforms where all loads are allowed to be unaligned, this kind of code only "happens to work" in the sense that the compiler was not "smart enough" to exploit the UB.

But yes, what I meant is that at the least, we need an escape hatch. We can't just make it entirely impossible to write such code.

But before we talk about a migration strategy, I think we should have some idea of what the goal is. Do we consider &x as *const _ to be a primitive operation (with its own MIR representation) that immediately creates a raw pointer? In that case, doing this to unaligned fields could even be a safe operation. That's probably pretty confusing though; let-expanding the code would then result in compiler errors or (worse) introduce UB. Or do we actually declare it legal for unsafe code to create an unaligned reference as long as they don't do much with it? But then, what are they allowed to do?

There is another case where we'd like people to use such a raw-ptr operation: Imagine we have a x: *const T which may not be aligned (because it could originate from a packed struct), and now we want to get a pointer to a field. &(*x).field would create an unaligned reference, which is a bad value and actually causes insta-UB under some proposed unsafe code guidelines. We'd rather have people write &(*x).field as *const _ (or whatever the primitive create-raw-ptr looks like) so that the bad reference is never even created.
However, I have no idea how we can enforce this. I imagine plenty of code around packed structs would be UB under this model.

(One reason we want it to be insta-UB is to allow more layout optimizations: Something like Option<Option<&i32>> could be ptr-sized if we could use the bit patterns for 0, 1, 2, 3 as discriminants. All of these are illegal for a 4-aligned pointer like &i32. But if we want to do this, we better make it illegal for there to ever be a reference that is not aligned.)

[RalfJung]

In the MaybeUninit discussion, another use case for immediately creating a raw pointer came up: Creating a pointer to an uninitialized field of a union. Creating a reference is invalid because it is not yet initialized, but creating a raw pointer is fine.

[retep998]

I just received this warning in winapi in case where it should not occur.

> cargo build --all-features
   Compiling winapi v0.3.4 (file:///C:/Users/Peter/Code/winapi-rs)
warning: #[derive] can't be used on a non-Copy #[repr(packed)] struct (error E0133)
   --> src\macros.rs:354:54
    |
354 |           STRUCT!{#[cfg_attr(feature = "debug", derive(Debug))] $($rest)*}
    |                                                        ^^^^^
    |
   ::: src\um\wingdi.rs:599:1
    |
599 | / STRUCT!{ #[debug] #[repr(packed)] struct BITMAPFILEHEADER {
600 | |     bfType: WORD,
601 | |     bfSize: DWORD,
602 | |     bfReserved1: WORD,
603 | |     bfReserved2: WORD,
604 | |     bfOffBits: DWORD,
605 | | }}
    | |__- in this macro invocation
    |
    = note: #[warn(safe_packed_borrows)] on by default
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

    Finished dev [unoptimized + debuginfo] target(s) in 33.10s

All structs defined by STRUCT! have manual Copy and Clone impls and should never trigger this warning, and yet the fact that is triggering indicates something is wrong with how the warning is implemented. Please fix this and don't you dare turn this into a hard error like this.

[RalfJung]

I reported a minimal example as https://github.com/rust-lang/rust/issues/50826

don't you dare turn this into a hard error like this.

Calm down, it's just a bug. That's why we have these warning cycles.

[ColinFinck]

For the record, declaring the borrowing of packed structure fields an unsafe operation makes Rust warn about getting the address of a packed structure field (like &fadt_table.x_dsdt as *const _ as usize), too.
However, getting a memory address is a totally safe operation.

Some may already conclude this from the previous comments, but this is the problem I'm currently facing.
Rust definitely needs a warning-free way for getting the address or a raw pointer to a packed structure field before this warning is turned into a hard error.

[gnzlbg]

Can't we just add an intrinsic that gives you a pointer to a value without going through a reference? Once we have that, if we really need sugar for this, we can add this later.

[hanna-kruppe]

Intrinsics are still functions so they can't move a value from the caller and then magically restore it to its original address and return that address. If we want this operation it must be a language concept.

However, since for legacy reasons &<something> as *const _ needs to be work anyway (at least when written exactly like that), the lint should really be updated to not warn on that pattern.

[RalfJung]

Rust definitely needs a warning-free way for getting the address or a raw pointer to a packed structure field before this warning is turned into a hard error.

It has such a way: Add an unsafe block.

We may want to have a safe way as well but IMHO that's much less critical.

[exscape]

I'm getting this warning despite deriving Copy and Clone.

extern crate serde;
extern crate serde_json;
#[macro_use] extern crate serde_derive;

#[repr(C, packed)]
#[derive(Copy, Clone, Serialize, Deserialize)]
struct Header {
    len: u16,
    kind: u8
}

fn main() {
}

Error message:

warning: #[derive] can't be used on a #[repr(packed)] struct that does not derive Copy (error E0133)
 --> src/main.rs:6:23
  |
6 | #[derive(Copy, Clone, Serialize, Deserialize)]
  |                       ^^^^^^^^^
  |
  = note: #[warn(safe_packed_borrows)] on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

This is on rustc 1.30.0-beta.8 (269c1bdc7 2018-09-27) with edition 2018. The error message above is from the Rust playground as of a few minutes prior to submitting this comment; I just hit "run", so the default toolchain (with the 2015 edition).

[retep998]

@exscape That's a bug with serde_derive. It isn't noticing the #[repr(packed)] and failing to generate a Serialize implementation that can work with packed fields.

[gnzlbg]

Is there a bug open for this in serde upstream?

[emilio]

This warns as well on bindgen for packed structs, but we cannot always derive copy and clone (since we don't want to allow copying, structs that contain incomplete arrays for example)... What's the way to go about that? Should the #[derive(Debug)] code learn about these?

See https://github.com/rust-lang-nursery/rust-bindgen/issues/1333

[cramertj]

@emilio It's not possible to implement Debug for non-Copy fields of a packed struct, since calling Debug::fmt on them requires creating an aligned reference to the field, which isn't possible when the field itself isn't Copy (and so can't be moved to a location w/ the proper alignment).

[retep998]

Of note is that I have since switched winapi to use derive(Copy) instead of impl Copy and that resolved the issue I was having earlier. Still unfortunate that it can't detect impl Copy but it's no longer a blocker on my end.

[cramertj]

@retep998 ah good to know! Thanks. Yeah, it is unfortunate.

[daniel5151]

Should this warning fire when attempting to #[derive] on a parameterized struct where only use of the parameters are with PhantomData members?

#[repr(packed)]
#[derive(Debug, Clone, Copy, PartialEq)]
struct Header<R> {
    request: u32,
    flags: u32,
    size: u32,
    _r: PhantomData<R>,
}

[RalfJung]

The warning is about the struct being packed, not about it having parameters.

Why is your struct packed? For a homogeneous struct of u32, repr(C) already has no padding.

[daniel5151]

Well, I ask the question because the following code compiles just fine:

#[repr(packed)]
#[derive(Debug, Clone, Copy, PartialEq)]
struct Header {
    request: u32,
    flags: u32,
    size: u32,
}

The only difference between the two structs being the presence of PhantomData.

The specific warning emitted for the PhantomData struct on 1.33.0 is
warning: #[derive] can't be used on a #[repr(packed)] struct with type parameters (error E0133), and links back to this issue.

And yes, I'm aware that marking this particular struct as packed doesn't matter. That said, it is supposed to match an evolving C interface that specifies a packed struct. Since the interface may change in the future to rely on packing behavior, I wanted to look into this issue now rather than ignoring it and potentially coming across it later.

[RalfJung]

Oh, sorry for this. I was not aware of this limitation of the derive macro.

[daniel5151]

No problem.
To rephrase my initial question though: if #[derive] works with regular packed structs, and PhantomData markers don't change the size / memory layout of structs, shouldn't #[derive] work with PhantomData marked structs?

[RalfJung]

I think (but I might be wrong) the problem is that derive runs very early in the compilation pipeline, before any kind of type checking or so. It would have a hard time even figuring out that a generic parameter is only used in a PhantomData.

I hope someone more knowledgeable can refine this guess.^^

[CJKay]

Could we introduce alignment guarantees to references/pointers, similar to how mut applies aliasing guarantees?

Something like:

#[repr(packed)]
struct X {
    p: u8,
    q: u32
}

fn main() {
    let x = X { p: 0, q: 0 };

    let y: &u32 = &x.q;
}

error[E0308]: mismatched types
  --> prog.rs:10:19
   |
10 |     let y: &u32 = &x.q;
   |                   ^^^^ types differ in alignment guarantees
   |
   = note: expected type `&u32`
              found type `&unalign u32`

Any &X could be implicitly convertible to &unalign X but not vice versa, meaning:

• Accesses to *(&X) would do what they do today
• Accesses to *(&unalign X) would generate mask-shift operations similarly to what I presume is done when accessing x.q directly

[retep998]

Would it be possible to have a lint for this, even in unsafe blocks? I'd like to be able to detect code that does borrows of packed fields to fix them, but much of that code would already be inside unsafe blocks and not detected by this warning.

[RalfJung]

@retep998 certainly, long-term the lint should be entirely independent of whether there is an unsafe block or not.

Shorter-term... I suppose we could already add that lint and make it allow-by-default, would that work?
I'm afraid a warn-by-default lint is probably not going to be received well when there's no stable alternative we can point people to.

[retep998]

An allow-by-default lint is perfectly fine for now. Even a clippy lint would be fine.

[RalfJung]

Lint is implemented in https://github.com/rust-lang/rust/pull/72270.

[geofft]

So, this diagnostic notices creating a reference/pointer to a misaligned field. But isn't initializing a misaligned field also UB?

#[repr(packed(2))]
struct Foo(u16, String);

let f = Foo(3, "hello!".to_owned());

seems to be accepted without complaint, and at least on x86_64, it seems to do an unaligned write.

[retep998]

No, that is not UB. Rust can easily see that you're assigning a value to a misaligned location and tell LLVM to generate the correct code for it. It is only the creation of references to misaligned locations that is bad because the reference does not preserve any alignment information.

[R030t1]

Hi, I'm encountering this from bindgen generated files. I've given the docs to bindgen a look and don't see a good way to modify the output. Is there a recommended solution for this case?