Skip to content

Allow &raw [mut | const] for union field in safe #141469

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
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
95 changes: 85 additions & 10 deletions compiler/rustc_mir_build/src/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ struct UnsafetyVisitor<'a, 'tcx> {
/// Flag to ensure that we only suggest wrapping the entire function body in
/// an unsafe block once.
suggest_unsafe_block: bool,
/// Controls how union field accesses are checked
union_field_access_mode: UnionFieldAccessMode,
}

impl<'tcx> UnsafetyVisitor<'_, 'tcx> {
Expand Down Expand Up @@ -223,6 +225,7 @@ impl<'tcx> UnsafetyVisitor<'_, 'tcx> {
inside_adt: false,
warnings: self.warnings,
suggest_unsafe_block: self.suggest_unsafe_block,
union_field_access_mode: UnionFieldAccessMode::Normal,
};
// params in THIR may be unsafe, e.g. a union pattern.
for param in &inner_thir.params {
Expand Down Expand Up @@ -545,6 +548,20 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
}
}
ExprKind::RawBorrow { arg, .. } => {
// Handle the case where we're taking a raw pointer to a union field
if let ExprKind::Scope { value: arg, .. } = self.thir[arg].kind {
if self.is_union_field_access(arg) {
// Taking a raw pointer to a union field is safe - just check the base expression
// but skip the union field safety check
self.visit_union_field_for_raw_borrow(arg);
return;
}
} else if self.is_union_field_access(arg) {
// Direct raw borrow of union field
self.visit_union_field_for_raw_borrow(arg);
return;
}

if let ExprKind::Scope { value: arg, .. } = self.thir[arg].kind
&& let ExprKind::Deref { arg } = self.thir[arg].kind
{
Expand Down Expand Up @@ -649,17 +666,27 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
if adt_def.variant(variant_index).fields[name].safety.is_unsafe() {
self.requires_unsafe(expr.span, UseOfUnsafeField);
} else if adt_def.is_union() {
if let Some(assigned_ty) = self.assignment_info {
if assigned_ty.needs_drop(self.tcx, self.typing_env) {
// This would be unsafe, but should be outright impossible since we
// reject such unions.
assert!(
self.tcx.dcx().has_errors().is_some(),
"union fields that need dropping should be impossible: {assigned_ty}"
);
// Check if this field access is part of a raw borrow operation
// If so, we've already handled it above and shouldn't reach here
match self.union_field_access_mode {
UnionFieldAccessMode::SuppressUnionFieldAccessError => {
// Suppress AccessToUnionField error for union fields chains
}
UnionFieldAccessMode::Normal => {
if let Some(assigned_ty) = self.assignment_info {
if assigned_ty.needs_drop(self.tcx, self.typing_env) {
// This would be unsafe, but should be outright impossible since we
// reject such unions.
assert!(
self.tcx.dcx().has_errors().is_some(),
"union fields that need dropping should be impossible: {assigned_ty}"
);
}
} else {
// Only require unsafe if this is not a raw borrow operation
self.requires_unsafe(expr.span, AccessToUnionField);
}
}
} else {
self.requires_unsafe(expr.span, AccessToUnionField);
}
}
}
Expand Down Expand Up @@ -712,6 +739,46 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
}
}

impl<'a, 'tcx> UnsafetyVisitor<'a, 'tcx> {
/// Check if an expression is a union field access
fn is_union_field_access(&self, expr_id: ExprId) -> bool {
match self.thir[expr_id].kind {
ExprKind::Field { lhs, .. } => {
let lhs = &self.thir[lhs];
matches!(lhs.ty.kind(), ty::Adt(adt_def, _) if adt_def.is_union())
}
_ => false,
}
}

/// Visit a union field access in the context of a raw borrow operation
/// This ensures we still check safety of nested operations while allowing
/// the raw pointer creation itself
fn visit_union_field_for_raw_borrow(&mut self, mut expr_id: ExprId) {
let prev = self.union_field_access_mode;
self.union_field_access_mode = UnionFieldAccessMode::SuppressUnionFieldAccessError;
// Walk through the chain of union field accesses using while let
while let ExprKind::Field { lhs, variant_index, name } = self.thir[expr_id].kind {
let lhs_expr = &self.thir[lhs];
if let ty::Adt(adt_def, _) = lhs_expr.ty.kind() {
// Check for unsafe fields but skip the union access check
if adt_def.variant(variant_index).fields[name].safety.is_unsafe() {
self.requires_unsafe(self.thir[expr_id].span, UseOfUnsafeField);
}
// If the LHS is also a union field access, keep walking
expr_id = lhs;
} else {
// Not a union, use normal visiting
visit::walk_expr(self, &self.thir[expr_id]);
return;
}
}
// Visit the base expression for any nested safety checks
self.visit_expr(&self.thir[expr_id]);
self.union_field_access_mode = prev;
}
}

#[derive(Clone)]
enum SafetyContext {
Safe,
Expand All @@ -720,6 +787,13 @@ enum SafetyContext {
UnsafeBlock { span: Span, hir_id: HirId, used: bool, nested_used_blocks: Vec<NestedUsedBlock> },
}

/// Controls how union field accesses are checked
#[derive(Clone, Copy)]
enum UnionFieldAccessMode {
Normal,
SuppressUnionFieldAccessError,
}

#[derive(Clone, Copy)]
struct NestedUsedBlock {
hir_id: HirId,
Expand Down Expand Up @@ -1199,6 +1273,7 @@ pub(crate) fn check_unsafety(tcx: TyCtxt<'_>, def: LocalDefId) {
inside_adt: false,
warnings: &mut warnings,
suggest_unsafe_block: true,
union_field_access_mode: UnionFieldAccessMode::Normal,
};
// params in THIR may be unsafe, e.g. a union pattern.
for param in &thir.params {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/tests/pass/both_borrows/smallvec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ impl<T, const N: usize> RawSmallVec<T, N> {
}

const fn as_mut_ptr_inline(&mut self) -> *mut T {
(unsafe { &raw mut self.inline }) as *mut T
&raw mut self.inline as *mut T
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed because it is not an unsafe operation anymore

const unsafe fn as_mut_ptr_heap(&mut self) -> *mut T {
Expand Down
57 changes: 57 additions & 0 deletions tests/ui/union/union-unsafe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ union U4<T: Copy> {
a: T,
}

union U5 {
a: usize,
}

union URef {
p: &'static mut i32,
}
Expand All @@ -31,6 +35,11 @@ fn deref_union_field(mut u: URef) {
*(u.p) = 13; //~ ERROR access to union field is unsafe
}

fn raw_deref_union_field(mut u: URef) {
// This is unsafe because we first dereference u.p (reading uninitialized memory)
let _p = &raw const *(u.p); //~ ERROR access to union field is unsafe
}

fn assign_noncopy_union_field(mut u: URefCell) {
u.a = (ManuallyDrop::new(RefCell::new(0)), 1); // OK (assignment does not drop)
u.a.0 = ManuallyDrop::new(RefCell::new(0)); // OK (assignment does not drop)
Expand All @@ -57,6 +66,20 @@ fn main() {
let a = u1.a; //~ ERROR access to union field is unsafe
u1.a = 11; // OK

let mut u2 = U1 { a: 10 };
let a = &raw mut u2.a; // OK
unsafe { *a = 3 };

let mut u3 = U1 { a: 10 };
let a = std::ptr::addr_of_mut!(u3.a); // OK
unsafe { *a = 14 };

let u4 = U5 { a: 2 };
let vec = vec![1, 2, 3];
// This is unsafe because we read u4.a (potentially uninitialized memory)
// to use as an array index
let _a = &raw const vec[u4.a]; //~ ERROR access to union field is unsafe

let U1 { a } = u1; //~ ERROR access to union field is unsafe
if let U1 { a: 12 } = u1 {} //~ ERROR access to union field is unsafe
if let Some(U1 { a: 13 }) = Some(u1) {} //~ ERROR access to union field is unsafe
Expand All @@ -73,4 +96,38 @@ fn main() {
let mut u3 = U3 { a: ManuallyDrop::new(String::from("old")) }; // OK
u3.a = ManuallyDrop::new(String::from("new")); // OK (assignment does not drop)
*u3.a = String::from("new"); //~ ERROR access to union field is unsafe

let mut unions = [U1 { a: 1 }, U1 { a: 2 }];

// Array indexing + union field raw borrow - should be OK
let ptr = &raw mut unions[0].a; // OK
let ptr2 = &raw const unions[1].a; // OK

// Test for union fields chain, this should be allowed
#[derive(Copy, Clone)]
union Inner {
a: u8,
}

union MoreInner {
moreinner: ManuallyDrop<Inner>,
}

union LessOuter {
lessouter: ManuallyDrop<MoreInner>,
}

union Outer {
outer: ManuallyDrop<LessOuter>,
}

let super_outer = Outer {
outer: ManuallyDrop::new(LessOuter {
lessouter: ManuallyDrop::new(MoreInner {
moreinner: ManuallyDrop::new(Inner { a: 42 }),
}),
}),
};

let ptr = &raw const super_outer.outer.lessouter.moreinner.a;
}
38 changes: 27 additions & 11 deletions tests/ui/union/union-unsafe.stderr
Original file line number Diff line number Diff line change
@@ -1,83 +1,99 @@
error[E0133]: access to union field is unsafe and requires unsafe function or block
--> $DIR/union-unsafe.rs:31:6
--> $DIR/union-unsafe.rs:35:6
|
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]: access to union field is unsafe and requires unsafe function or block
--> $DIR/union-unsafe.rs:43:6
--> $DIR/union-unsafe.rs:40:26
|
LL | let _p = &raw const *(u.p);
| ^^^^^ 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:52: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:49:6
--> $DIR/union-unsafe.rs:58: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
--> $DIR/union-unsafe.rs:66: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
--> $DIR/union-unsafe.rs:81:29
|
LL | let _a = &raw const vec[u4.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:83: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
--> $DIR/union-unsafe.rs:84: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:62:25
--> $DIR/union-unsafe.rs:85:25
|
LL | if let Some(U1 { a: 13 }) = Some(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:67:6
--> $DIR/union-unsafe.rs:90: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:71:6
--> $DIR/union-unsafe.rs:94: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:75:6
--> $DIR/union-unsafe.rs:98: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 10 previous errors
error: aborting due to 12 previous errors

For more information about this error, try `rustc --explain E0133`.
Loading