Skip to content

Commit

Permalink
make unaligned_reference a hard error
Browse files Browse the repository at this point in the history
  • Loading branch information
RalfJung committed Jan 31, 2023
1 parent f361413 commit dfc4a7b
Show file tree
Hide file tree
Showing 27 changed files with 170 additions and 686 deletions.
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

0 comments on commit dfc4a7b

Please sign in to comment.