From 64856e29c1c780117348c196e05902476251bc92 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 24 Oct 2020 21:12:32 +0200 Subject: [PATCH] adjust union access unsafety check logic to take into account Deref and the actual type of the assignment --- .../rustc_mir/src/transform/check_unsafety.rs | 35 +++--- src/test/ui/union/union-unsafe.rs | 21 ++++ src/test/ui/union/union-unsafe.stderr | 110 +++++++++++------- 3 files changed, 108 insertions(+), 58 deletions(-) diff --git a/compiler/rustc_mir/src/transform/check_unsafety.rs b/compiler/rustc_mir/src/transform/check_unsafety.rs index fdc033f9eb3f..e78f8d4c901d 100644 --- a/compiler/rustc_mir/src/transform/check_unsafety.rs +++ b/compiler/rustc_mir/src/transform/check_unsafety.rs @@ -190,7 +190,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { } } - for (i, elem) in place.projection.iter().enumerate() { + for (i, _elem) in place.projection.iter().enumerate() { let proj_base = &place.projection[..i]; if context.is_borrow() { if util::is_disaligned(self.tcx, self.body, self.param_env, *place) { @@ -236,23 +236,28 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { UnsafetyViolationDetails::DerefOfRawPointer, ), ty::Adt(adt, _) if adt.is_union() => { - if context == PlaceContext::MutatingUse(MutatingUseContext::Store) + let assign_to_field = context + == PlaceContext::MutatingUse(MutatingUseContext::Store) || context == PlaceContext::MutatingUse(MutatingUseContext::Drop) - || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) - { - let elem_ty = match elem { - ProjectionElem::Field(_, ty) => ty, - _ => span_bug!( - self.source_info.span, - "non-field projection {:?} from union?", - place - ), - }; - let manually_drop = elem_ty + || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput); + // If there is a `Deref` further along the projection chain, this is *not* an + // assignment to a union field. In that case the union field is just read to + // obtain the pointer/reference. + let assign_to_field = assign_to_field + && !place.projection[i..] + .iter() + .any(|elem| matches!(elem, ProjectionElem::Deref)); + // If this is just an assignment, determine if the assigned type needs dropping. + if assign_to_field { + // We have to check the actual type of the assignment, as that determines if the + // old value is being dropped. + let assigned_ty = place.ty(&self.body.local_decls, self.tcx).ty; + // To avoid semver hazard, we only consider `Copy` and `ManuallyDrop` non-dropping. + let manually_drop = assigned_ty .ty_adt_def() .map_or(false, |adt_def| adt_def.is_manually_drop()); let nodrop = manually_drop - || elem_ty.is_copy_modulo_regions( + || assigned_ty.is_copy_modulo_regions( self.tcx.at(self.source_info.span), self.param_env, ); @@ -260,7 +265,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { self.require_unsafe( UnsafetyViolationKind::GeneralAndConstFn, UnsafetyViolationDetails::AssignToDroppingUnionField, - ) + ); } else { // write to non-drop union field, safe } diff --git a/src/test/ui/union/union-unsafe.rs b/src/test/ui/union/union-unsafe.rs index 9810ed75d591..9c7ed1f7bfc3 100644 --- a/src/test/ui/union/union-unsafe.rs +++ b/src/test/ui/union/union-unsafe.rs @@ -1,4 +1,6 @@ +#![feature(untagged_unions)] use std::mem::ManuallyDrop; +use std::cell::RefCell; union U1 { a: u8 @@ -16,6 +18,25 @@ union U4 { a: T } +union URef { + p: &'static mut i32, +} + +union URefCell { // field that does not drop but is not `Copy`, either + a: (RefCell, i32), +} + +fn deref_union_field(mut u: URef) { + // Not an assignment but an access to the union field! + *(u.p) = 13; //~ ERROR access to union field is unsafe +} + +fn assign_noncopy_union_field(mut u: URefCell) { + u.a = (RefCell::new(0), 1); //~ ERROR assignment to union field that needs dropping + u.a.0 = RefCell::new(0); //~ ERROR assignment to union field that needs dropping + u.a.1 = 1; // OK +} + fn generic_noncopy() { let mut u3 = U3 { a: ManuallyDrop::new(T::default()) }; u3.a = ManuallyDrop::new(T::default()); // OK (assignment does not drop) diff --git a/src/test/ui/union/union-unsafe.stderr b/src/test/ui/union/union-unsafe.stderr index 5b3a58814a93..f80d4394f842 100644 --- a/src/test/ui/union/union-unsafe.stderr +++ b/src/test/ui/union/union-unsafe.stderr @@ -1,67 +1,91 @@ error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:22:6 + --> $DIR/union-unsafe.rs:31:5 + | +LL | *(u.p) = 13; + | ^^^^^^^^^^^ access to union field + | + = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior + +error[E0133]: assignment to union field that needs dropping is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:35:5 + | +LL | u.a = (RefCell::new(0), 1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that needs dropping + | + = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized + +error[E0133]: assignment to union field that needs dropping is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:36:5 + | +LL | u.a.0 = RefCell::new(0); + | ^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that needs dropping + | + = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized + +error[E0133]: access to union field is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:43:6 | LL | *u3.a = T::default(); | ^^^^ access to union field | = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior -error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:28:6 - | -LL | *u3.a = T::default(); - | ^^^^ access to union field - | - = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior - -error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:36:13 - | -LL | let a = u1.a; - | ^^^^ access to union field - | - = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior - -error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:39:14 - | -LL | let U1 { a } = u1; - | ^ access to union field - | - = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior - -error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:40:20 - | -LL | if let U1 { a: 12 } = u1 {} - | ^^ access to union field - | - = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior - -error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:45:6 - | -LL | *u2.a = String::from("new"); - | ^^^^ access to union field - | - = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior - error[E0133]: access to union field is unsafe and requires unsafe function or block --> $DIR/union-unsafe.rs:49:6 | +LL | *u3.a = T::default(); + | ^^^^ access to union field + | + = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior + +error[E0133]: access to union field is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:57:13 + | +LL | let a = u1.a; + | ^^^^ access to union field + | + = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior + +error[E0133]: access to union field is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:60:14 + | +LL | let U1 { a } = u1; + | ^ access to union field + | + = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior + +error[E0133]: access to union field is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:61:20 + | +LL | if let U1 { a: 12 } = u1 {} + | ^^ access to union field + | + = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior + +error[E0133]: access to union field is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:66:6 + | +LL | *u2.a = String::from("new"); + | ^^^^ access to union field + | + = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior + +error[E0133]: access to union field is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:70:6 + | LL | *u3.a = 1; | ^^^^ access to union field | = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:53:6 + --> $DIR/union-unsafe.rs:74:6 | LL | *u3.a = String::from("new"); | ^^^^ access to union field | = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior -error: aborting due to 8 previous errors +error: aborting due to 11 previous errors For more information about this error, try `rustc --explain E0133`.