Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make unaligned_reference a hard error #102513

Merged
merged 1 commit into from
Feb 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion compiler/rustc_error_codes/src/error_codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// /!\ IMPORTANT /!\
//
// Error messages' format must follow the RFC 1567 available here:
// https://github.com/rust-lang/rfcs/pull/1567
// https://rust-lang.github.io/rfcs/1567-long-error-codes-explanation-normalization.html

register_diagnostics! {
E0001: include_str!("./error_codes/E0001.md"),
Expand Down Expand Up @@ -510,6 +510,7 @@ E0789: include_str!("./error_codes/E0789.md"),
E0790: include_str!("./error_codes/E0790.md"),
E0791: include_str!("./error_codes/E0791.md"),
E0792: include_str!("./error_codes/E0792.md"),
E0793: include_str!("./error_codes/E0793.md"),
;
// E0006, // merged with E0005
// E0008, // cannot bind by-move into a pattern guard
Expand Down
64 changes: 64 additions & 0 deletions compiler/rustc_error_codes/src/error_codes/E0793.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
An unaligned references to a field of a [packed] struct got created.

Erroneous code example:

```compile_fail,E0793
#[repr(packed)]
pub struct Foo {
field1: u64,
field2: u8,
}

unsafe {
let foo = Foo { field1: 0, field2: 0 };
// Accessing the field directly is fine.
let val = foo.field1;
// A reference to a packed field causes a error.
let val = &foo.field1; // ERROR
// An implicit `&` is added in format strings, causing the same error.
println!("{}", foo.field1); // ERROR
}
```

Creating a reference to an insufficiently aligned packed field is
[undefined behavior] and therefore disallowed. Using an `unsafe` block does not
change anything about this. Instead, the code should do a copy of the data in
the packed field or use raw pointers and unaligned accesses.

```
#[repr(packed)]
pub struct Foo {
field1: u64,
field2: u8,
}

unsafe {
let foo = Foo { field1: 0, field2: 0 };

// Instead of a reference, we can create a raw pointer...
let ptr = std::ptr::addr_of!(foo.field1);
// ... and then (crucially!) access it in an explicitly unaligned way.
let val = unsafe { ptr.read_unaligned() };
// This would *NOT* be correct:
// let val = unsafe { *ptr }; // Undefined Behavior due to unaligned load!

// For formatting, we can create a copy to avoid the direct reference.
let copy = foo.field1;
println!("{}", copy);
// Creating a copy can be written in a single line with curly braces.
// (This is equivalent to the two lines above.)
println!("{}", { foo.field1 });
}
```

### Additional information

Note that this error is specifically about *references* to packed fields.
Direct by-value access of those fields is fine, since then the compiler has
enough information to generate the correct kind of access.

See [issue #82523] for more information.

[packed]: https://doc.rust-lang.org/reference/type-layout.html#the-alignment-modifiers
[undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html
[issue #82523]: https://github.com/rust-lang/rust/issues/82523
11 changes: 10 additions & 1 deletion compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,6 @@ fn register_builtins(store: &mut LintStore) {
store.register_renamed("exceeding_bitshifts", "arithmetic_overflow");
store.register_renamed("redundant_semicolon", "redundant_semicolons");
store.register_renamed("overlapping_patterns", "overlapping_range_endpoints");
store.register_renamed("safe_packed_borrows", "unaligned_references");
store.register_renamed("disjoint_capture_migration", "rust_2021_incompatible_closure_captures");
store.register_renamed("or_patterns_back_compat", "rust_2021_incompatible_or_patterns");
store.register_renamed("non_fmt_panic", "non_fmt_panics");
Expand Down Expand Up @@ -487,6 +486,16 @@ fn register_builtins(store: &mut LintStore) {
"converted into hard error, see issue #71800 \
<https://github.com/rust-lang/rust/issues/71800> for more information",
);
store.register_removed(
"safe_packed_borrows",
"converted into hard error, see issue #82523 \
<https://github.com/rust-lang/rust/issues/82523> for more information",
);
store.register_removed(
"unaligned_references",
"converted into hard error, see issue #82523 \
<https://github.com/rust-lang/rust/issues/82523> for more information",
);
}

fn register_internals(store: &mut LintStore) {
Expand Down
46 changes: 0 additions & 46 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1187,51 +1187,6 @@ declare_lint! {
"lints that have been renamed or removed"
}

declare_lint! {
/// The `unaligned_references` lint detects unaligned references to fields
/// of [packed] structs.
///
/// [packed]: https://doc.rust-lang.org/reference/type-layout.html#the-alignment-modifiers
///
/// ### Example
///
/// ```rust,compile_fail
/// #[repr(packed)]
/// pub struct Foo {
/// field1: u64,
/// field2: u8,
/// }
///
/// fn main() {
/// unsafe {
/// let foo = Foo { field1: 0, field2: 0 };
/// let _ = &foo.field1;
/// println!("{}", foo.field1); // An implicit `&` is added here, triggering the lint.
/// }
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Creating a reference to an insufficiently aligned packed field is [undefined behavior] and
/// should be disallowed. Using an `unsafe` block does not change anything about this. Instead,
/// the code should do a copy of the data in the packed field or use raw pointers and unaligned
/// accesses. See [issue #82523] for more information.
///
/// [undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html
/// [issue #82523]: https://github.com/rust-lang/rust/issues/82523
pub UNALIGNED_REFERENCES,
Deny,
"detects unaligned references to fields of packed structs",
@future_incompatible = FutureIncompatibleInfo {
reference: "issue #82523 <https://github.com/rust-lang/rust/issues/82523>",
reason: FutureIncompatibilityReason::FutureReleaseErrorReportNow,
};
report_in_external_macro
}

declare_lint! {
/// The `const_item_mutation` lint detects attempts to mutate a `const`
/// item.
Expand Down Expand Up @@ -3308,7 +3263,6 @@ declare_lint_pass! {
PUB_USE_OF_PRIVATE_EXTERN_CRATE,
INVALID_TYPE_PARAM_DEFAULT,
RENAMED_AND_REMOVED_LINTS,
UNALIGNED_REFERENCES,
CONST_ITEM_MUTATION,
PATTERNS_IN_FNS_WITHOUT_BODY,
MISSING_FRAGMENT_SPECIFIER,
Expand Down
43 changes: 17 additions & 26 deletions compiler/rustc_mir_transform/src/check_packed_ref.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use rustc_errors::struct_span_err;
use rustc_middle::mir::visit::{PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::{self, TyCtxt};
use rustc_session::lint::builtin::UNALIGNED_REFERENCES;

use crate::util;
use crate::MirLint;
Expand Down Expand Up @@ -49,31 +49,22 @@ impl<'tcx> Visitor<'tcx> for PackedRefChecker<'_, 'tcx> {
// shouldn't do.
unreachable!();
} else {
let source_info = self.source_info;
let lint_root = self.body.source_scopes[source_info.scope]
.local_data
.as_ref()
.assert_crate_local()
.lint_root;
self.tcx.struct_span_lint_hir(
UNALIGNED_REFERENCES,
lint_root,
source_info.span,
"reference to packed field is unaligned",
|lint| {
lint
.note(
"fields of packed structs are not properly aligned, and creating \
a misaligned reference is undefined behavior (even if that \
reference is never dereferenced)",
)
.help(
"copy the field contents to a local variable, or replace the \
reference with a raw pointer and use `read_unaligned`/`write_unaligned` \
(loads and stores via `*p` must be properly aligned even when using raw pointers)"
)
},
);
struct_span_err!(
self.tcx.sess,
self.source_info.span,
E0793,
"reference to packed field is unaligned"
)
.note(
"fields of packed structs are not properly aligned, and creating \
a misaligned reference is undefined behavior (even if that \
reference is never dereferenced)",
).help(
"copy the field contents to a local variable, or replace the \
reference with a raw pointer and use `read_unaligned`/`write_unaligned` \
(loads and stores via `*p` must be properly aligned even when using raw pointers)"
)
.emit();
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,26 @@
// This should fail even without validation/SB
//@compile-flags: -Zmiri-disable-validation -Zmiri-disable-stacked-borrows

#![allow(dead_code, unused_variables, unaligned_references)]
#![allow(dead_code, unused_variables)]

use std::{ptr, mem};

#[repr(packed)]
struct Foo {
x: i32,
y: i32,
}

unsafe fn raw_to_ref<'a, T>(x: *const T) -> &'a T {
mem::transmute(x)
}

fn main() {
// Try many times as this might work by chance.
for _ in 0..20 {
let foo = Foo { x: 42, y: 99 };
let p = &foo.x;
let i = *p; //~ERROR: alignment 4 is required
// There seem to be implicit reborrows, which make the error already appear here
let p: &i32 = unsafe { raw_to_ref(ptr::addr_of!(foo.x)) }; //~ERROR: alignment 4 is required
let i = *p;
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: Undefined Behavior: accessing memory with alignment ALIGN, but alignment ALIGN is required
--> $DIR/reference_to_packed.rs:LL:CC
|
LL | let i = *p;
| ^^ accessing memory with alignment ALIGN, but alignment ALIGN is required
LL | let p: &i32 = unsafe { raw_to_ref(ptr::addr_of!(foo.x)) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ accessing memory with alignment ALIGN, but alignment ALIGN is required
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
Expand Down
4 changes: 0 additions & 4 deletions tests/ui/binding/issue-53114-safety-checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,11 @@ fn let_wild_gets_unsafe_field() {
let u2 = U { a: I(1) };
let p = P { a: &2, b: &3 };
let _ = &p.b; //~ ERROR reference to packed field
//~^ WARN will become a hard error
let _ = u1.a; // #53114: should eventually signal error as well
let _ = &u2.a; //~ ERROR [E0133]

// variation on above with `_` in substructure
let (_,) = (&p.b,); //~ ERROR reference to packed field
//~^ WARN will become a hard error
let (_,) = (u1.a,); //~ ERROR [E0133]
let (_,) = (&u2.a,); //~ ERROR [E0133]
}
Expand All @@ -37,13 +35,11 @@ fn match_unsafe_field_to_wild() {
let u2 = U { a: I(1) };
let p = P { a: &2, b: &3 };
match &p.b { _ => { } } //~ ERROR reference to packed field
//~^ WARN will become a hard error
match u1.a { _ => { } } //~ ERROR [E0133]
match &u2.a { _ => { } } //~ ERROR [E0133]

// variation on above with `_` in substructure
match (&p.b,) { (_,) => { } } //~ ERROR reference to packed field
//~^ WARN will become a hard error
match (u1.a,) { (_,) => { } } //~ ERROR [E0133]
match (&u2.a,) { (_,) => { } } //~ ERROR [E0133]
}
Expand Down
Loading