regression: unaligned references to packed fields are now an error #109745

Mark-Simulacrum · 2023-03-30T00:26:51Z

This is an expected strengthening of a future compatibility lint, mostly filing an issue for tracking purposes (i.e., release notes). I believe no action is expected.

apiraino · 2023-03-30T11:09:25Z

ok, then I will remove the I-prioritize flag, too. IIUC, these changes will be in the release notes and crates will need to adapt to the new lints

@rustbot label -I-prioritize

glandium · 2023-04-05T05:51:47Z

Having hit this one in https://github.com/mozilla/authenticator-rs/blob/abde4645a4711e29f1a481802776d5079c559eb1/src/windows/winapi.rs#L224, I'm wondering if this new error is not too restrictive.
Here is a smaller testcase that is not x86 windows specific:

#[repr(packed)]
pub struct Foo {
    a: usize,
    b: [u16; 1],
}

pub fn foo(f: &Foo) {
    bar(f.b.as_ptr());
}

pub fn bar(_ptr: *const u16) {}

How can accessing the b field ever be unaligned?

Mark-Simulacrum · 2023-04-05T06:13:34Z

Cc @RalfJung @rust-lang/lang (also nominated for lang discussion; this will hit stable in ~2 weeks)

FWIW it seems like that example is a little misleading, the as_ptr call takes &self, so just &f.b as the argument is equivalent.

repr(packed) doesn't alter field reordering by itself I think (and the reference seems to agree), so the [u16; 1] could be at the front of the struct.

But adding repr(C) doesn't prevent the error, and I think with that this would only be unaligned on 8-bit platforms (which I'm not sure we support; I'm also not sure if this error is target specific, I assume so). My guess is that the implementation isn't trying to be smart about this, just always errors on any field with lowered alignment requirements as a result of packed.

glandium · 2023-04-05T06:23:02Z

so just &f.b as the argument is equivalent.

it's actually not, because bar takes a *const u16, and &f.b is a [u16; 1]. .as_ptr() returns a *const u16 (per https://doc.rust-lang.org/std/primitive.slice.html#method.as_ptr)

FWIW the original error happens on x86 windows only because that's windows specific code, and only x86 has repr(packed) on the relevant struct. It also does have a #[repr(C)] that was hidden in a macro in winapi, which I had missed.

Mark-Simulacrum · 2023-04-05T06:34:02Z

The return type doesn't seem relevant? as_ptr has &self, so for the purposes of the error what happens after that doesn't matter; bar isn't relevant. (It would change the type of the expression to move to &, and stop compiling if that's the only change made)

scottmcm · 2023-04-05T06:46:44Z

How can accessing the b field ever be unaligned?

I'm not following what you mean by this example.

I switched the & to addr_of to make it compile, put a read of the pointer in bar as a way to test alignment, then wrote some basic entirely safe code to demonstrate that it absolutely can end up unaligned:

error: Undefined Behavior: accessing memory with alignment 1, but alignment 2 is required
  --> src/main.rs:13:14
   |
13 |     unsafe { ptr.read() };
   |              ^^^^^^^^^^ accessing memory with alignment 1, but alignment 2 is required

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=246744a5d3066b205afeb097bbcf053e

glandium · 2023-04-05T06:52:24Z

The return type doesn't seem relevant? as_ptr has &self, so for the purposes of the error what happens after that doesn't matter; bar isn't relevant. (It would change the type of the expression to move to &, and stop compiling if that's the only change made)

Sure, for E0793, the return type is irrelevant.

How can accessing the b field ever be unaligned?

I'm not following what you mean by this example.

I switched the & to addr_of to make it compile, put a read of the pointer in bar as a way to test alignment, then wrote some basic entirely safe code to demonstrate that it absolutely can end up unaligned:
error: Undefined Behavior: accessing memory with alignment 1, but alignment 2 is required
  --> src/main.rs:13:14
   |
13 |     unsafe { ptr.read() };
   |              ^^^^^^^^^^ accessing memory with alignment 1, but alignment 2 is required
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=246744a5d3066b205afeb097bbcf053e

Add repr(C) to the struct as mentioned in the more recent messages.

glandium · 2023-04-05T06:53:59Z

~~Interestingly, miri doesn't barf if you use addr_of!(f.b[0]).~~

Scrap that, it does.

scottmcm · 2023-04-05T06:55:05Z

Add repr(C) to the struct as mentioned in the more recent messages.

Can you give a playground link with exactly what you mean? I added repr(C) to Foo and it's still unaligned: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2b904e588e84c7eee280194f56766c57

JakobDegen · 2023-04-05T07:02:50Z

@glandium in this example:

#[repr(packed)]
pub struct Foo {
    a: usize,
    b: [u16; 1],
}

pub fn foo(f: &Foo) {
    bar(f.b.as_ptr());
}

pub fn bar(_ptr: *const u16) {}

The alignment of Foo is 1. This might be a bit counterintuitive since it's not how alignment usually works. If you make the alignment 8 then you indeed don't get an error.

#[repr(packed(8))]
pub struct Foo {
    a: usize,
    b: [u16; 1],
}

pub fn foo(f: &Foo) {
    bar(f.b.as_ptr());
}

pub fn bar(_ptr: *const u16) {}

glandium · 2023-04-05T07:02:57Z

I might have hit the wrong button. I see what's going on in your example. It's actually surprising that repr(packed) also means repr(align(1)). Considering that, yes, E0793 is unfortunately right...

RalfJung · 2023-04-06T01:54:08Z

Wow that's a long list of regressions, in particular given that we did a crater run before ramping up the warning to an error.

It's actually surprising that repr(packed) also means repr(align(1)).

I agree, but this has been the case since Rust 1.0. AFAIK this is to be compatible with the corresponding annotation provided by C compilers.

tmandry · 2023-04-11T19:08:30Z

@rust-lang/lang team discussed.

We don't have blocking concerns and trust the people involved in this that it's time to ship: from what we could tell, the future incompat lint has been around for over 2 years, and the original issue was from 2015, and it's a deny-by-default lint on stable. (In the future, we'd prefer to have links to the relevant history for nominated issues like this.) The breaking changes are justified by this and the fact that the code was already UB anyway.

RalfJung · 2024-04-25T05:51:27Z

There's nothing actionable here as far as I can tell, and the regressions are an expected part of fixing the soundness bug. Thus, closing.

Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Mar 30, 2023

Mark-Simulacrum added this to the 1.69.0 milestone Mar 30, 2023

rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 30, 2023

Mark-Simulacrum mentioned this issue Mar 30, 2023

Crater runs for 1.69.0 #109310

Closed

rustbot removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 30, 2023

Mark-Simulacrum added the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 5, 2023

pnkfelix mentioned this issue Apr 11, 2023

When erroring on inadequate alignment, consider reminding reader that packed can lower alignment #110199

Closed

scottmcm removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 18, 2023

Mark-Simulacrum added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels May 26, 2023

rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 26, 2023

BoxyUwU removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 26, 2023

RalfJung closed this as completed Apr 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

regression: unaligned references to packed fields are now an error #109745

regression: unaligned references to packed fields are now an error #109745

Mark-Simulacrum commented Mar 30, 2023

apiraino commented Mar 30, 2023

glandium commented Apr 5, 2023

Mark-Simulacrum commented Apr 5, 2023

glandium commented Apr 5, 2023

Mark-Simulacrum commented Apr 5, 2023

scottmcm commented Apr 5, 2023

glandium commented Apr 5, 2023

glandium commented Apr 5, 2023 •

edited

Loading

scottmcm commented Apr 5, 2023

JakobDegen commented Apr 5, 2023

glandium commented Apr 5, 2023

RalfJung commented Apr 6, 2023

tmandry commented Apr 11, 2023

RalfJung commented Apr 25, 2024

regression: unaligned references to packed fields are now an error #109745

regression: unaligned references to packed fields are now an error #109745

Comments

Mark-Simulacrum commented Mar 30, 2023

apiraino commented Mar 30, 2023

glandium commented Apr 5, 2023

Mark-Simulacrum commented Apr 5, 2023

glandium commented Apr 5, 2023

Mark-Simulacrum commented Apr 5, 2023

scottmcm commented Apr 5, 2023

glandium commented Apr 5, 2023

glandium commented Apr 5, 2023 • edited Loading

scottmcm commented Apr 5, 2023

JakobDegen commented Apr 5, 2023

glandium commented Apr 5, 2023

RalfJung commented Apr 6, 2023

tmandry commented Apr 11, 2023

RalfJung commented Apr 25, 2024

glandium commented Apr 5, 2023 •

edited

Loading