Closed Mark-Simulacrum closed 6 months ago
apiraino
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago How can accessing the
bfield 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 requiredThe 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
bfield ever be unaligned?I'm not following what you mean by this example.
I switched the
&toaddr_ofto make it compile, put a read of the pointer inbaras 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
Add repr(C) to the struct as mentioned in the more recent messages.
glandium
commented
1 year ago ~Interestingly, miri doesn't barf if you use addr_of!(f.b[0]).~
Scrap that, it does.
scottmcm
commented
1 year ago 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
commented
1 year ago @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) {}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
commented
1 year ago 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
commented
1 year ago @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
commented
6 months ago 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.
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.