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

consider assignments of union field of ManuallyDrop type safe #78068

Merged
merged 7 commits into from
Dec 15, 2020
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
15 changes: 15 additions & 0 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1741,6 +1741,21 @@ impl<'tcx> Place<'tcx> {
pub fn as_ref(&self) -> PlaceRef<'tcx> {
PlaceRef { local: self.local, projection: &self.projection }
}

/// Iterate over the projections in evaluation order, i.e., the first element is the base with
/// its projection and then subsequently more projections are added.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to give an example based on Rust code. For example:

Given the place a.b.c, this would yield:

  • (a, b)
  • (a.b, c)

I am a bit surprised by this structure -- I guess I expected it to return a, a.b, and a.b.c, rather than a tuple, and to have people match on the "tail" projection (if any). But I guess this is ok too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expanded the comment.

I expected it to return a, a.b, and a.b.c, rather than a tuple, and to have people match on the "tail" projection (if any). But I guess this is ok too.

I first thought of something like this, but it doesn't really match what clients need, at least what this particular client needs. The point is to check the projections, so the iterator really should yield as often as there are projections. And given that it also seemed odd to not make the projection itself directly available.

In a follow-up PR I hope to port more clients to this API, I guess then we will see how generally useful it is.

/// As a concrete example, given the place a.b.c, this would yield:
/// - (a, .b)
/// - (a.b, .c)
/// Given a place without projections, the iterator is empty.
pub fn iter_projections(
self,
) -> impl Iterator<Item = (PlaceRef<'tcx>, PlaceElem<'tcx>)> + DoubleEndedIterator {
self.projection.iter().enumerate().map(move |(i, proj)| {
let base = PlaceRef { local: self.local, projection: &self.projection[..i] };
(base, proj)
})
}
}

impl From<Local> for Place<'_> {
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_middle/src/mir/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pub enum UnsafetyViolationDetails {
UseOfMutableStatic,
UseOfExternStatic,
DerefOfRawPointer,
AssignToNonCopyUnionField,
AssignToDroppingUnionField,
AccessToUnionField,
MutationOfLayoutConstrainedField,
BorrowOfLayoutConstrainedField,
Expand Down Expand Up @@ -94,8 +94,8 @@ impl UnsafetyViolationDetails {
"raw pointers may be NULL, dangling or unaligned; they can violate aliasing rules \
and cause data races: all of these are undefined behavior",
),
AssignToNonCopyUnionField => (
"assignment to non-`Copy` union field",
AssignToDroppingUnionField => (
"assignment to union field that might need dropping",
"the previous content of the field will be dropped, which causes undefined \
behavior if the field was not properly initialized",
),
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_middle/src/mir/tcx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,15 @@ impl<'tcx> Place<'tcx> {
}
}

impl<'tcx> PlaceRef<'tcx> {
pub fn ty<D>(&self, local_decls: &D, tcx: TyCtxt<'tcx>) -> PlaceTy<'tcx>
where
D: HasLocalDecls<'tcx>,
{
Place::ty_from(self.local, &self.projection, local_decls, tcx)
}
}

pub enum RvalueInitializationState {
Shallow,
Deep,
Expand Down
165 changes: 93 additions & 72 deletions compiler/rustc_mir/src/transform/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
self.check_mut_borrowing_layout_constrained_field(*place, context.is_mutating_use());
}

// Check for borrows to packed fields.
// `is_disaligned` already traverses the place to consider all projections after the last
// `Deref`, so this only needs to be called once at the top level.
if context.is_borrow() {
if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
self.require_unsafe(
Expand All @@ -190,87 +193,105 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
}
}

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) {
// Some checks below need the extra metainfo of the local declaration.
let decl = &self.body.local_decls[place.local];

// Check the base local: it might be an unsafe-to-access static. We only check derefs of the
// temporary holding the static pointer to avoid duplicate errors
// <https://github.com/rust-lang/rust/pull/78068#issuecomment-731753506>.
if decl.internal && place.projection.first() == Some(&ProjectionElem::Deref) {
// If the projection root is an artifical local that we introduced when
// desugaring `static`, give a more specific error message
// (avoid the general "raw pointer" clause below, that would only be confusing).
if let Some(box LocalInfo::StaticRef { def_id, .. }) = decl.local_info {
if self.tcx.is_mutable_static(def_id) {
self.require_unsafe(
UnsafetyViolationKind::BorrowPacked,
UnsafetyViolationDetails::BorrowOfPackedField,
UnsafetyViolationKind::General,
UnsafetyViolationDetails::UseOfMutableStatic,
);
return;
} else if self.tcx.is_foreign_item(def_id) {
self.require_unsafe(
UnsafetyViolationKind::General,
UnsafetyViolationDetails::UseOfExternStatic,
);
return;
}
}
let source_info = self.source_info;
if let [] = proj_base {
let decl = &self.body.local_decls[place.local];
if decl.internal {
// If the projection root is an artifical local that we introduced when
// desugaring `static`, give a more specific error message
// (avoid the general "raw pointer" clause below, that would only be confusing).
if let Some(box LocalInfo::StaticRef { def_id, .. }) = decl.local_info {
if self.tcx.is_mutable_static(def_id) {
self.require_unsafe(
UnsafetyViolationKind::General,
UnsafetyViolationDetails::UseOfMutableStatic,
);
return;
} else if self.tcx.is_foreign_item(def_id) {
self.require_unsafe(
UnsafetyViolationKind::General,
UnsafetyViolationDetails::UseOfExternStatic,
);
return;
}
} else {
// Internal locals are used in the `move_val_init` desugaring.
// We want to check unsafety against the source info of the
// desugaring, rather than the source info of the RHS.
self.source_info = self.body.local_decls[place.local].source_info;
}
}

// Check for raw pointer `Deref`.
for (base, proj) in place.iter_projections() {
if proj == ProjectionElem::Deref {
let source_info = self.source_info; // Backup source_info so we can restore it later.
if base.projection.is_empty() && decl.internal {
// Internal locals are used in the `move_val_init` desugaring.
// We want to check unsafety against the source info of the
// desugaring, rather than the source info of the RHS.
self.source_info = self.body.local_decls[place.local].source_info;
}
let base_ty = base.ty(self.body, self.tcx).ty;
if base_ty.is_unsafe_ptr() {
self.require_unsafe(
UnsafetyViolationKind::GeneralAndConstFn,
UnsafetyViolationDetails::DerefOfRawPointer,
)
}
self.source_info = source_info; // Restore backed-up source_info.
}
let base_ty = Place::ty_from(place.local, proj_base, self.body, self.tcx).ty;
match base_ty.kind() {
ty::RawPtr(..) => self.require_unsafe(
UnsafetyViolationKind::GeneralAndConstFn,
UnsafetyViolationDetails::DerefOfRawPointer,
),
ty::Adt(adt, _) => {
if adt.is_union() {
if 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
),
};
if !elem_ty.is_copy_modulo_regions(
self.tcx.at(self.source_info.span),
self.param_env,
) {
self.require_unsafe(
UnsafetyViolationKind::GeneralAndConstFn,
UnsafetyViolationDetails::AssignToNonCopyUnionField,
)
} else {
// write to non-move union, safe
}
} else {
self.require_unsafe(
UnsafetyViolationKind::GeneralAndConstFn,
UnsafetyViolationDetails::AccessToUnionField,
)
}
}

// Check for union fields. For this we traverse right-to-left, as the last `Deref` changes
// whether we *read* the union field or potentially *write* to it (if this place is being assigned to).
let mut saw_deref = false;
for (base, proj) in place.iter_projections().rev() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, cute, I missed that this was a double-ended iterator at first and wondered how you were going to manage this

if proj == ProjectionElem::Deref {
saw_deref = true;
continue;
}

let base_ty = base.ty(self.body, self.tcx).ty;
if base_ty.ty_adt_def().map_or(false, |adt| adt.is_union()) {
// If we did not hit a `Deref` yet and the overall place use is an assignment, the
// rules are different.
let assign_to_field = !saw_deref
&& matches!(
context,
PlaceContext::MutatingUse(
MutatingUseContext::Store
| MutatingUseContext::Drop
| MutatingUseContext::AsmOutput
)
);
// 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
|| assigned_ty.is_copy_modulo_regions(
self.tcx.at(self.source_info.span),
self.param_env,
);
if !nodrop {
self.require_unsafe(
UnsafetyViolationKind::GeneralAndConstFn,
UnsafetyViolationDetails::AssignToDroppingUnionField,
);
} else {
// write to non-drop union field, safe
}
} else {
self.require_unsafe(
UnsafetyViolationKind::GeneralAndConstFn,
UnsafetyViolationDetails::AccessToUnionField,
)
}
_ => {}
}
self.source_info = source_info;
}
}
}
Expand Down
27 changes: 24 additions & 3 deletions src/test/ui/union/union-unsafe.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#![feature(untagged_unions)]
use std::mem::ManuallyDrop;
use std::cell::RefCell;

union U1 {
a: u8
Expand All @@ -16,9 +18,28 @@ union U4<T: Copy> {
a: T
}

union URef {
p: &'static mut i32,
}

union URefCell { // field that does not drop but is not `Copy`, either
a: (RefCell<i32>, 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 might need dropping
u.a.0 = RefCell::new(0); //~ ERROR assignment to union field that might need dropping
u.a.1 = 1; // OK
}

fn generic_noncopy<T: Default>() {
let mut u3 = U3 { a: ManuallyDrop::new(T::default()) };
u3.a = ManuallyDrop::new(T::default()); //~ ERROR assignment to non-`Copy` union field is unsafe
u3.a = ManuallyDrop::new(T::default()); // OK (assignment does not drop)
*u3.a = T::default(); //~ ERROR access to union field is unsafe
}

Expand All @@ -41,14 +62,14 @@ fn main() {
// let U1 { .. } = u1; // OK

let mut u2 = U2 { a: ManuallyDrop::new(String::from("old")) }; // OK
u2.a = ManuallyDrop::new(String::from("new")); //~ ERROR assignment to non-`Copy` union
u2.a = ManuallyDrop::new(String::from("new")); // OK (assignment does not drop)
*u2.a = String::from("new"); //~ ERROR access to union field is unsafe

let mut u3 = U3 { a: ManuallyDrop::new(0) }; // OK
u3.a = ManuallyDrop::new(1); // OK
*u3.a = 1; //~ ERROR access to union field is unsafe

let mut u3 = U3 { a: ManuallyDrop::new(String::from("old")) }; // OK
u3.a = ManuallyDrop::new(String::from("new")); //~ ERROR assignment to non-`Copy` union
u3.a = ManuallyDrop::new(String::from("new")); // OK (assignment does not drop)
*u3.a = String::from("new"); //~ ERROR access to union field is unsafe
}
Loading