From 612a9b2f95fc44dd140a3545dc303058d98100a9 Mon Sep 17 00:00:00 2001 From: Aman Arora Date: Sat, 6 Mar 2021 21:47:53 -0500 Subject: [PATCH] 2229: Handle capturing a reference into a repr packed struct RFC 1240 states that it is unsafe to capture references into a packed-struct. This PR ensures that when a closure captures a precise path, we aren't violating this safety constraint. To acheive so we restrict the capture precision to the struct itself. An interesting edge case: ```rust struct Foo(String); let foo: Foo; let c = || { println!("{}", foo.0); let x = foo.0; } ``` Given how closures get desugared today, foo.0 will be moved into the closure, making the `println!`, safe. However this can be very subtle and also will be unsafe if the closure gets inline. Closes: https://github.com/rust-lang/project-rfc-2229/issues/33 --- compiler/rustc_typeck/src/check/upvar.rs | 61 ++++++- .../diagnostics/repr_packed.rs | 36 ++++ .../diagnostics/repr_packed.stderr | 22 +++ .../2229_closure_analysis/repr_packed.rs | 103 +++++++++++ .../2229_closure_analysis/repr_packed.stderr | 165 ++++++++++++++++++ 5 files changed, 381 insertions(+), 6 deletions(-) create mode 100644 src/test/ui/closures/2229_closure_analysis/diagnostics/repr_packed.rs create mode 100644 src/test/ui/closures/2229_closure_analysis/diagnostics/repr_packed.stderr create mode 100644 src/test/ui/closures/2229_closure_analysis/repr_packed.rs create mode 100644 src/test/ui/closures/2229_closure_analysis/repr_packed.stderr diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs index 69c09528662..254f12b9563 100644 --- a/compiler/rustc_typeck/src/check/upvar.rs +++ b/compiler/rustc_typeck/src/check/upvar.rs @@ -1053,6 +1053,52 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } } +/// Truncate the capture so that the place being borrowed is in accordance with RFC 1240, +/// which states that it's unsafe to take a reference into a struct marked `repr(packed)`. +fn restrict_repr_packed_field_ref_capture<'tcx>( + tcx: TyCtxt<'tcx>, + param_env: ty::ParamEnv<'tcx>, + place: &Place<'tcx>, +) -> Place<'tcx> { + let pos = place.projections.iter().enumerate().position(|(i, p)| { + let ty = place.ty_before_projection(i); + + // Return true for fields of packed structs, unless those fields have alignment 1. + match p.kind { + ProjectionKind::Field(..) => match ty.kind() { + ty::Adt(def, _) if def.repr.packed() => { + match tcx.layout_raw(param_env.and(p.ty)) { + Ok(layout) if layout.align.abi.bytes() == 1 => { + // if the alignment is 1, the type can't be further + // disaligned. + debug!( + "restrict_repr_packed_field_ref_capture: ({:?}) - align = 1", + place + ); + false + } + _ => { + debug!("restrict_repr_packed_field_ref_capture: ({:?}) - true", place); + true + } + } + } + + _ => false, + }, + _ => false, + } + }); + + let mut place = place.clone(); + + if let Some(pos) = pos { + place.projections.truncate(pos); + } + + place +} + struct InferBorrowKind<'a, 'tcx> { fcx: &'a FnCtxt<'a, 'tcx>, @@ -1391,8 +1437,15 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> { place_with_id, diag_expr_id, bk ); + let place = restrict_repr_packed_field_ref_capture( + self.fcx.tcx, + self.fcx.param_env, + &place_with_id.place, + ); + let place_with_id = PlaceWithHirId { place, ..*place_with_id }; + if !self.capture_information.contains_key(&place_with_id.place) { - self.init_capture_info_for_place(place_with_id, diag_expr_id); + self.init_capture_info_for_place(&place_with_id, diag_expr_id); } match bk { @@ -1409,11 +1462,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> { fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId) { debug!("mutate(assignee_place={:?}, diag_expr_id={:?})", assignee_place, diag_expr_id); - if !self.capture_information.contains_key(&assignee_place.place) { - self.init_capture_info_for_place(assignee_place, diag_expr_id); - } - - self.adjust_upvar_borrow_kind_for_mut(assignee_place, diag_expr_id); + self.borrow(assignee_place, diag_expr_id, ty::BorrowKind::MutBorrow); } } diff --git a/src/test/ui/closures/2229_closure_analysis/diagnostics/repr_packed.rs b/src/test/ui/closures/2229_closure_analysis/diagnostics/repr_packed.rs new file mode 100644 index 00000000000..6fce2951505 --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/diagnostics/repr_packed.rs @@ -0,0 +1,36 @@ +// check-pass + +#![feature(capture_disjoint_fields)] +//~^ WARNING: the feature `capture_disjoint_fields` is incomplete + +// Given how the closure desugaring is implemented (at least at the time of writing this test), +// we don't need to truncate the captured path to a reference into a packed-struct if the field +// being referenced will be moved into the closure, since it's safe to move out a field from a +// packed-struct. +// +// However to avoid surprises for the user, or issues when the closure is +// inlined we will truncate the capture to access just the struct regardless of if the field +// might get moved into the closure. +// +// It is possible for someone to try writing the code that relies on the desugaring to access a ref +// into a packed-struct without explicity using unsafe. Here we test that the compiler warns the +// user that such an access is still unsafe. +fn test_missing_unsafe_warning_on_repr_packed() { + #[repr(packed)] + struct Foo { x: String } + + let foo = Foo { x: String::new() }; + + let c = || { + println!("{}", foo.x); + //~^ WARNING: borrow of packed field is unsafe and requires unsafe function or block + //~| WARNING: this was previously accepted by the compiler but is being phased out + let _z = foo.x; + }; + + c(); +} + +fn main() { + test_missing_unsafe_warning_on_repr_packed(); +} diff --git a/src/test/ui/closures/2229_closure_analysis/diagnostics/repr_packed.stderr b/src/test/ui/closures/2229_closure_analysis/diagnostics/repr_packed.stderr new file mode 100644 index 00000000000..440b2c54c0a --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/diagnostics/repr_packed.stderr @@ -0,0 +1,22 @@ +warning: the feature `capture_disjoint_fields` is incomplete and may not be safe to use and/or cause compiler crashes + --> $DIR/repr_packed.rs:3:12 + | +LL | #![feature(capture_disjoint_fields)] + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `#[warn(incomplete_features)]` on by default + = note: see issue #53488 for more information + +warning: borrow of packed field is unsafe and requires unsafe function or block (error E0133) + --> $DIR/repr_packed.rs:25:24 + | +LL | println!("{}", foo.x); + | ^^^^^ + | + = 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 + = note: fields of packed structs might be misaligned: dereferencing a misaligned pointer or even just creating a misaligned reference is undefined behavior + +warning: 2 warnings emitted + diff --git a/src/test/ui/closures/2229_closure_analysis/repr_packed.rs b/src/test/ui/closures/2229_closure_analysis/repr_packed.rs new file mode 100644 index 00000000000..2b9ef2a76bb --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/repr_packed.rs @@ -0,0 +1,103 @@ +#![feature(capture_disjoint_fields)] +//~^ WARNING: the feature `capture_disjoint_fields` is incomplete +//~| `#[warn(incomplete_features)]` on by default +//~| see issue #53488 + +#![feature(rustc_attrs)] + +// `u8` aligned at a byte and are unaffected by repr(packed). +// Therefore we can precisely (and safely) capture references to both the fields. +fn test_alignment_not_affected() { + #[repr(packed)] + struct Foo { x: u8, y: u8 } + + let mut foo = Foo { x: 0, y: 0 }; + + let mut c = #[rustc_capture_analysis] + //~^ ERROR: attributes on expressions are experimental + //~| NOTE: see issue #15701 + || { + //~^ ERROR: First Pass analysis includes: + //~| ERROR: Min Capture analysis includes: + let z1: &u8 = &foo.x; + //~^ NOTE: Capturing foo[(0, 0)] -> ImmBorrow + //~| NOTE: Min Capture foo[(0, 0)] -> ImmBorrow + let z2: &mut u8 = &mut foo.y; + //~^ NOTE: Capturing foo[(1, 0)] -> MutBorrow + //~| NOTE: Min Capture foo[(1, 0)] -> MutBorrow + + *z2 = 42; + + println!("({}, {})", z1, z2); + }; + + c(); +} + +// `String`, `u16` are not aligned at a one byte boundry and are thus affected by repr(packed). +// +// Here we test that the closure doesn't capture a reference point to `foo.x` but +// rather capture `foo` entirely. +fn test_alignment_affected() { + #[repr(packed)] + struct Foo { x: String, y: u16 } + + let mut foo = Foo { x: String::new(), y: 0 }; + + let mut c = #[rustc_capture_analysis] + //~^ ERROR: attributes on expressions are experimental + //~| NOTE: see issue #15701 + || { + //~^ ERROR: First Pass analysis includes: + //~| ERROR: Min Capture analysis includes: + let z1: &String = &foo.x; + let z2: &mut u16 = &mut foo.y; + //~^ NOTE: Capturing foo[] -> MutBorrow + //~| NOTE: Min Capture foo[] -> MutBorrow + + + *z2 = 42; + + println!("({}, {})", z1, z2); + }; + + c(); +} + +// Given how the closure desugaring is implemented (at least at the time of writing this test), +// we don't need to truncate the captured path to a reference into a packed-struct if the field +// being referenced will be moved into the closure, since it's safe to move out a field from a +// packed-struct. +// +// However to avoid surprises for the user, or issues when the closure is +// inlined we will truncate the capture to access just the struct regardless of if the field +// might get moved into the closure. +fn test_truncation_when_ref_and_move() { + #[repr(packed)] + struct Foo { x: String } + + let mut foo = Foo { x: String::new() }; + + let c = #[rustc_capture_analysis] + //~^ ERROR: attributes on expressions are experimental + //~| NOTE: see issue #15701 + || { + //~^ ERROR: First Pass analysis includes: + //~| ERROR: Min Capture analysis includes: + println!("{}", foo.x); + //~^ NOTE: Capturing foo[] -> ImmBorrow + //~| NOTE: Min Capture foo[] -> ByValue + //~| NOTE: foo[] used here + let _z = foo.x; + //~^ NOTE: Capturing foo[(0, 0)] -> ByValue + //~| NOTE: foo[] captured as ByValue here + }; + + c(); +} + +fn main() { + test_truncation_when_ref_and_move(); + test_alignment_affected(); + test_alignment_not_affected(); +} diff --git a/src/test/ui/closures/2229_closure_analysis/repr_packed.stderr b/src/test/ui/closures/2229_closure_analysis/repr_packed.stderr new file mode 100644 index 00000000000..0517dd04b6f --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/repr_packed.stderr @@ -0,0 +1,165 @@ +error[E0658]: attributes on expressions are experimental + --> $DIR/repr_packed.rs:16:17 + | +LL | let mut c = #[rustc_capture_analysis] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: see issue #15701 for more information + = help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable + +error[E0658]: attributes on expressions are experimental + --> $DIR/repr_packed.rs:47:17 + | +LL | let mut c = #[rustc_capture_analysis] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: see issue #15701 for more information + = help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable + +error[E0658]: attributes on expressions are experimental + --> $DIR/repr_packed.rs:81:13 + | +LL | let c = #[rustc_capture_analysis] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: see issue #15701 for more information + = help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable + +warning: the feature `capture_disjoint_fields` is incomplete and may not be safe to use and/or cause compiler crashes + --> $DIR/repr_packed.rs:1:12 + | +LL | #![feature(capture_disjoint_fields)] + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `#[warn(incomplete_features)]` on by default + = note: see issue #53488 for more information + +error: First Pass analysis includes: + --> $DIR/repr_packed.rs:19:5 + | +LL | / || { +LL | | +LL | | +LL | | let z1: &u8 = &foo.x; +... | +LL | | println!("({}, {})", z1, z2); +LL | | }; + | |_____^ + | +note: Capturing foo[(0, 0)] -> ImmBorrow + --> $DIR/repr_packed.rs:22:24 + | +LL | let z1: &u8 = &foo.x; + | ^^^^^ +note: Capturing foo[(1, 0)] -> MutBorrow + --> $DIR/repr_packed.rs:25:32 + | +LL | let z2: &mut u8 = &mut foo.y; + | ^^^^^ + +error: Min Capture analysis includes: + --> $DIR/repr_packed.rs:19:5 + | +LL | / || { +LL | | +LL | | +LL | | let z1: &u8 = &foo.x; +... | +LL | | println!("({}, {})", z1, z2); +LL | | }; + | |_____^ + | +note: Min Capture foo[(0, 0)] -> ImmBorrow + --> $DIR/repr_packed.rs:22:24 + | +LL | let z1: &u8 = &foo.x; + | ^^^^^ +note: Min Capture foo[(1, 0)] -> MutBorrow + --> $DIR/repr_packed.rs:25:32 + | +LL | let z2: &mut u8 = &mut foo.y; + | ^^^^^ + +error: First Pass analysis includes: + --> $DIR/repr_packed.rs:50:5 + | +LL | / || { +LL | | +LL | | +LL | | let z1: &String = &foo.x; +... | +LL | | println!("({}, {})", z1, z2); +LL | | }; + | |_____^ + | +note: Capturing foo[] -> MutBorrow + --> $DIR/repr_packed.rs:54:33 + | +LL | let z2: &mut u16 = &mut foo.y; + | ^^^^^ + +error: Min Capture analysis includes: + --> $DIR/repr_packed.rs:50:5 + | +LL | / || { +LL | | +LL | | +LL | | let z1: &String = &foo.x; +... | +LL | | println!("({}, {})", z1, z2); +LL | | }; + | |_____^ + | +note: Min Capture foo[] -> MutBorrow + --> $DIR/repr_packed.rs:54:33 + | +LL | let z2: &mut u16 = &mut foo.y; + | ^^^^^ + +error: First Pass analysis includes: + --> $DIR/repr_packed.rs:84:5 + | +LL | / || { +LL | | +LL | | +LL | | println!("{}", foo.x); +... | +LL | | +LL | | }; + | |_____^ + | +note: Capturing foo[] -> ImmBorrow + --> $DIR/repr_packed.rs:87:24 + | +LL | println!("{}", foo.x); + | ^^^^^ +note: Capturing foo[(0, 0)] -> ByValue + --> $DIR/repr_packed.rs:91:18 + | +LL | let _z = foo.x; + | ^^^^^ + +error: Min Capture analysis includes: + --> $DIR/repr_packed.rs:84:5 + | +LL | / || { +LL | | +LL | | +LL | | println!("{}", foo.x); +... | +LL | | +LL | | }; + | |_____^ + | +note: Min Capture foo[] -> ByValue + --> $DIR/repr_packed.rs:87:24 + | +LL | println!("{}", foo.x); + | ^^^^^ foo[] used here +... +LL | let _z = foo.x; + | ^^^^^ foo[] captured as ByValue here + +error: aborting due to 9 previous errors; 1 warning emitted + +For more information about this error, try `rustc --explain E0658`.