Skip to content

Commit 74d0d74

Browse files
syvbsyvb
committed
Check for union field accesses in THIR unsafeck
1 parent 969a6c2 commit 74d0d74

File tree

69 files changed

+1083
-71
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

69 files changed

+1083
-71
lines changed

‎compiler/rustc_mir_build/src/check_unsafety.rs

+127-3
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ struct UnsafetyVisitor<'a, 'tcx> {
2626
/// calls to functions with `#[target_feature]` (RFC 2396).
2727
body_target_features: &'tcx Vec<Symbol>,
2828
is_const: bool,
29+
in_possible_lhs_union_assign: bool,
30+
in_union_destructure: bool,
2931
}
3032

3133
impl<'tcx> UnsafetyVisitor<'_, 'tcx> {
@@ -158,14 +160,115 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
158160
}
159161
}
160162

163+
fn visit_pat(&mut self, pat: &Pat<'tcx>) {
164+
use PatKind::*;
165+
166+
if self.in_union_destructure {
167+
match *pat.kind {
168+
// binding to a variable allows getting stuff out of variable
169+
Binding { .. }
170+
// match is conditional on having this value
171+
| Constant { .. }
172+
| Variant { .. }
173+
| Leaf { .. }
174+
| Deref { .. }
175+
| Range { .. }
176+
| Slice { .. }
177+
| Array { .. } => {
178+
self.requires_unsafe(pat.span, AccessToUnionField);
179+
return; // don't walk pattern
180+
}
181+
// wildcard doesn't take anything
182+
Wild |
183+
// these just wrap other patterns
184+
Or { .. } |
185+
AscribeUserType { .. } => {}
186+
}
187+
};
188+
189+
if let ty::Adt(adt_def, _) = pat.ty.kind() {
190+
// check for extracting values from union via destructuring
191+
if adt_def.is_union() {
192+
match *pat.kind {
193+
// assigning the whole union is okay
194+
// let x = Union { ... };
195+
// let y = x; // safe
196+
Binding { .. } |
197+
// binding to wildcard is okay since that never reads anything and stops double errors
198+
// with implict wildcard branches from `if let`s
199+
Wild |
200+
// doesn't have any effect on semantics
201+
AscribeUserType { .. } |
202+
// creating a union literal
203+
Constant { .. } => {},
204+
Variant { .. } | Leaf { .. } | Or { .. } => {
205+
// pattern matching with a union and not doing something like v = Union { bar: 5 }
206+
self.in_union_destructure = true;
207+
visit::walk_pat(self, pat);
208+
self.in_union_destructure = false;
209+
return; // don't walk pattern
210+
}
211+
Deref { .. } | Range { .. } | Slice { .. } | Array { .. } =>
212+
unreachable!("impossible union destructuring type"),
213+
}
214+
}
215+
}
216+
217+
visit::walk_pat(self, pat);
218+
}
219+
161220
fn visit_expr(&mut self, expr: &Expr<'tcx>) {
221+
// could we be in a the LHS of an assignment of a union?
222+
match expr.kind {
223+
ExprKind::Field { .. }
224+
| ExprKind::VarRef { .. }
225+
| ExprKind::UpvarRef { .. }
226+
| ExprKind::Scope { .. }
227+
| ExprKind::Cast { .. } => {}
228+
229+
ExprKind::AddressOf { .. }
230+
| ExprKind::Adt { .. }
231+
| ExprKind::Array { .. }
232+
| ExprKind::Binary { .. }
233+
| ExprKind::Block { .. }
234+
| ExprKind::Borrow { .. }
235+
| ExprKind::Literal { .. }
236+
| ExprKind::ConstBlock { .. }
237+
| ExprKind::Deref { .. }
238+
| ExprKind::Index { .. }
239+
| ExprKind::NeverToAny { .. }
240+
| ExprKind::PlaceTypeAscription { .. }
241+
| ExprKind::ValueTypeAscription { .. }
242+
| ExprKind::Pointer { .. }
243+
| ExprKind::Repeat { .. }
244+
| ExprKind::StaticRef { .. }
245+
| ExprKind::ThreadLocalRef { .. }
246+
| ExprKind::Tuple { .. }
247+
| ExprKind::Unary { .. }
248+
| ExprKind::Call { .. }
249+
| ExprKind::Assign { .. }
250+
| ExprKind::AssignOp { .. }
251+
| ExprKind::Break { .. }
252+
| ExprKind::Closure { .. }
253+
| ExprKind::Continue { .. }
254+
| ExprKind::Return { .. }
255+
| ExprKind::Yield { .. }
256+
| ExprKind::Loop { .. }
257+
| ExprKind::Match { .. }
258+
| ExprKind::Box { .. }
259+
| ExprKind::If { .. }
260+
| ExprKind::InlineAsm { .. }
261+
| ExprKind::LlvmInlineAsm { .. }
262+
| ExprKind::LogicalOp { .. }
263+
| ExprKind::Use { .. } => self.in_possible_lhs_union_assign = false,
264+
};
162265
match expr.kind {
163266
ExprKind::Scope { value, lint_level: LintLevel::Explicit(hir_id), region_scope: _ } => {
164267
let prev_id = self.hir_context;
165268
self.hir_context = hir_id;
166269
self.visit_expr(&self.thir[value]);
167270
self.hir_context = prev_id;
168-
return;
271+
return; // don't visit the whole expression
169272
}
170273
ExprKind::Call { fun, ty: _, args: _, from_hir_call: _, fn_span: _ } => {
171274
if self.thir[fun].ty.fn_sig(self.tcx).unsafety() == hir::Unsafety::Unsafe {
@@ -246,9 +349,29 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
246349
// Unsafe blocks can be used in closures, make sure to take it into account
247350
self.safety_context = closure_visitor.safety_context;
248351
}
352+
ExprKind::Field { lhs, .. } => {
353+
// assigning to union field is okay for AccessToUnionField
354+
if let ty::Adt(adt_def, _) = &self.thir[lhs].ty.kind() {
355+
if adt_def.is_union() {
356+
if self.in_possible_lhs_union_assign {
357+
// FIXME: trigger AssignToDroppingUnionField unsafety if needed
358+
} else {
359+
self.requires_unsafe(expr.span, AccessToUnionField);
360+
}
361+
}
362+
}
363+
}
364+
// don't have any special handling for AssignOp since it causes a read *and* write to lhs
365+
ExprKind::Assign { lhs, rhs } => {
366+
// assigning to a union is safe, check here so it doesn't get treated as a read later
367+
self.in_possible_lhs_union_assign = true;
368+
visit::walk_expr(self, &self.thir()[lhs]);
369+
self.in_possible_lhs_union_assign = false;
370+
visit::walk_expr(self, &self.thir()[rhs]);
371+
return; // don't visit the whole expression
372+
}
249373
_ => {}
250374
}
251-
252375
visit::walk_expr(self, expr);
253376
}
254377
}
@@ -296,7 +419,6 @@ enum UnsafeOpKind {
296419
DerefOfRawPointer,
297420
#[allow(dead_code)] // FIXME
298421
AssignToDroppingUnionField,
299-
#[allow(dead_code)] // FIXME
300422
AccessToUnionField,
301423
#[allow(dead_code)] // FIXME
302424
MutationOfLayoutConstrainedField,
@@ -417,6 +539,8 @@ pub fn check_unsafety<'tcx>(tcx: TyCtxt<'tcx>, def: ty::WithOptConstParam<LocalD
417539
body_unsafety,
418540
body_target_features,
419541
is_const,
542+
in_possible_lhs_union_assign: false,
543+
in_union_destructure: false,
420544
};
421545
visitor.visit_expr(&thir[expr]);
422546
}

‎compiler/rustc_mir_build/src/thir/visit.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,8 @@ pub fn walk_expr<'a, 'tcx: 'a, V: Visitor<'a, 'tcx>>(visitor: &mut V, expr: &Exp
153153
}
154154

155155
pub fn walk_stmt<'a, 'tcx: 'a, V: Visitor<'a, 'tcx>>(visitor: &mut V, stmt: &Stmt<'tcx>) {
156-
match stmt.kind {
157-
StmtKind::Expr { expr, scope: _ } => visitor.visit_expr(&visitor.thir()[expr]),
156+
match &stmt.kind {
157+
StmtKind::Expr { expr, scope: _ } => visitor.visit_expr(&visitor.thir()[*expr]),
158158
StmtKind::Let {
159159
initializer,
160160
remainder_scope: _,
@@ -163,7 +163,7 @@ pub fn walk_stmt<'a, 'tcx: 'a, V: Visitor<'a, 'tcx>>(visitor: &mut V, stmt: &Stm
163163
lint_level: _,
164164
} => {
165165
if let Some(init) = initializer {
166-
visitor.visit_expr(&visitor.thir()[init]);
166+
visitor.visit_expr(&visitor.thir()[*init]);
167167
}
168168
visitor.visit_pat(pattern);
169169
}

‎src/test/ui/union/union-align.rs

+3
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
// run-pass
2+
// revisions: mirunsafeck thirunsafeck
3+
// [thirunsafeck]compile-flags: -Z thir-unsafeck
4+
25
#![allow(dead_code)]
36

47
use std::mem::{size_of, size_of_val, align_of, align_of_val};

‎src/test/ui/union/union-backcomp.rs

+3
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
// run-pass
2+
// revisions: mirunsafeck thirunsafeck
3+
// [thirunsafeck]compile-flags: -Z thir-unsafeck
4+
25
#![allow(path_statements)]
36
#![allow(dead_code)]
47

‎src/test/ui/union/union-basic.rs

+3
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
// run-pass
2+
// revisions: mirunsafeck thirunsafeck
3+
// [thirunsafeck]compile-flags: -Z thir-unsafeck
4+
25
#![allow(unused_imports)]
36

47
// aux-build:union.rs

‎src/test/ui/union/union-borrow-move-parent-sibling.stderr renamed to ‎src/test/ui/union/union-borrow-move-parent-sibling.mirunsafeck.stderr

+5-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error[E0502]: cannot borrow `u` (via `u.y`) as immutable because it is also borrowed as mutable (via `u.x.0`)
2-
--> $DIR/union-borrow-move-parent-sibling.rs:53:13
2+
--> $DIR/union-borrow-move-parent-sibling.rs:56:13
33
|
44
LL | let a = &mut u.x.0;
55
| ---------- mutable borrow occurs here (via `u.x.0`)
@@ -11,7 +11,7 @@ LL | use_borrow(a);
1111
= note: `u.y` is a field of the union `U`, so it overlaps the field `u.x.0`
1212

1313
error[E0382]: use of moved value: `u`
14-
--> $DIR/union-borrow-move-parent-sibling.rs:60:13
14+
--> $DIR/union-borrow-move-parent-sibling.rs:63:13
1515
|
1616
LL | let u = U { x: ((MockVec::new(), MockVec::new()), MockVec::new()) };
1717
| - move occurs because `u` has type `U`, which does not implement the `Copy` trait
@@ -21,7 +21,7 @@ LL | let b = u.y;
2121
| ^^^ value used here after move
2222

2323
error[E0502]: cannot borrow `u` (via `u.y`) as immutable because it is also borrowed as mutable (via `u.x.0.0`)
24-
--> $DIR/union-borrow-move-parent-sibling.rs:66:13
24+
--> $DIR/union-borrow-move-parent-sibling.rs:69:13
2525
|
2626
LL | let a = &mut (u.x.0).0;
2727
| -------------- mutable borrow occurs here (via `u.x.0.0`)
@@ -33,7 +33,7 @@ LL | use_borrow(a);
3333
= note: `u.y` is a field of the union `U`, so it overlaps the field `u.x.0.0`
3434

3535
error[E0382]: use of moved value: `u`
36-
--> $DIR/union-borrow-move-parent-sibling.rs:73:13
36+
--> $DIR/union-borrow-move-parent-sibling.rs:76:13
3737
|
3838
LL | let u = U { x: ((MockVec::new(), MockVec::new()), MockVec::new()) };
3939
| - move occurs because `u` has type `U`, which does not implement the `Copy` trait
@@ -43,7 +43,7 @@ LL | let b = u.y;
4343
| ^^^ value used here after move
4444

4545
error[E0502]: cannot borrow `u` (via `u.x`) as immutable because it is also borrowed as mutable (via `u.y`)
46-
--> $DIR/union-borrow-move-parent-sibling.rs:79:13
46+
--> $DIR/union-borrow-move-parent-sibling.rs:82:13
4747
|
4848
LL | let a = &mut *u.y;
4949
| --- mutable borrow occurs here (via `u.y`)

‎src/test/ui/union/union-borrow-move-parent-sibling.rs

+3
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
// revisions: mirunsafeck thirunsafeck
2+
// [thirunsafeck]compile-flags: -Z thir-unsafeck
3+
14
#![feature(untagged_unions)]
25
#![allow(unused)]
36

‎src/test/ui/union/union-borrow-move-parent-sibling.thirunsafeck.stderr

+60
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
error[E0502]: cannot borrow `u` (via `u.y`) as immutable because it is also borrowed as mutable (via `u.x.0`)
2+
--> $DIR/union-borrow-move-parent-sibling.rs:56:13
3+
|
4+
LL | let a = &mut u.x.0;
5+
| ---------- mutable borrow occurs here (via `u.x.0`)
6+
LL | let b = &u.y;
7+
| ^^^^ immutable borrow of `u.y` -- which overlaps with `u.x.0` -- occurs here
8+
LL | use_borrow(a);
9+
| - mutable borrow later used here
10+
|
11+
= note: `u.y` is a field of the union `U`, so it overlaps the field `u.x.0`
12+
13+
error[E0382]: use of moved value: `u`
14+
--> $DIR/union-borrow-move-parent-sibling.rs:63:13
15+
|
16+
LL | let u = U { x: ((MockVec::new(), MockVec::new()), MockVec::new()) };
17+
| - move occurs because `u` has type `U`, which does not implement the `Copy` trait
18+
LL | let a = u.x.0;
19+
| ----- value moved here
20+
LL | let b = u.y;
21+
| ^^^ value used here after move
22+
23+
error[E0502]: cannot borrow `u` (via `u.y`) as immutable because it is also borrowed as mutable (via `u.x.0.0`)
24+
--> $DIR/union-borrow-move-parent-sibling.rs:69:13
25+
|
26+
LL | let a = &mut (u.x.0).0;
27+
| -------------- mutable borrow occurs here (via `u.x.0.0`)
28+
LL | let b = &u.y;
29+
| ^^^^ immutable borrow of `u.y` -- which overlaps with `u.x.0.0` -- occurs here
30+
LL | use_borrow(a);
31+
| - mutable borrow later used here
32+
|
33+
= note: `u.y` is a field of the union `U`, so it overlaps the field `u.x.0.0`
34+
35+
error[E0382]: use of moved value: `u`
36+
--> $DIR/union-borrow-move-parent-sibling.rs:76:13
37+
|
38+
LL | let u = U { x: ((MockVec::new(), MockVec::new()), MockVec::new()) };
39+
| - move occurs because `u` has type `U`, which does not implement the `Copy` trait
40+
LL | let a = (u.x.0).0;
41+
| --------- value moved here
42+
LL | let b = u.y;
43+
| ^^^ value used here after move
44+
45+
error[E0502]: cannot borrow `u` (via `u.x`) as immutable because it is also borrowed as mutable (via `u.y`)
46+
--> $DIR/union-borrow-move-parent-sibling.rs:82:13
47+
|
48+
LL | let a = &mut *u.y;
49+
| --- mutable borrow occurs here (via `u.y`)
50+
LL | let b = &u.x;
51+
| ^^^^ immutable borrow of `u.x` -- which overlaps with `u.y` -- occurs here
52+
LL | use_borrow(a);
53+
| - mutable borrow later used here
54+
|
55+
= note: `u.x` is a field of the union `U`, so it overlaps the field `u.y`
56+
57+
error: aborting due to 5 previous errors
58+
59+
Some errors have detailed explanations: E0382, E0502.
60+
For more information about an error, try `rustc --explain E0382`.

‎src/test/ui/union/union-const-codegen.rs

+2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
// run-pass
2+
// revisions: mirunsafeck thirunsafeck
3+
// [thirunsafeck]compile-flags: -Z thir-unsafeck
24

35
union U {
46
a: u64,

‎src/test/ui/union/union-const-eval-field.rs

+2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
// run-pass
2+
// revisions: mirunsafeck thirunsafeck
3+
// [thirunsafeck]compile-flags: -Z thir-unsafeck
24

35
type Field1 = (i32, u32);
46
type Field2 = f32;

‎src/test/ui/union/union-const-eval.rs

+3
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
// build-pass (FIXME(62277): could be check-pass?)
2+
// revisions: mirunsafeck thirunsafeck
3+
// [thirunsafeck]compile-flags: -Z thir-unsafeck
4+
25
#![feature(const_fn_union)]
36

47
union U {

0 commit comments

Comments
 (0)