Skip to content

Commit 212d74f

Browse files
borsbors
committed
Auto merge of #45436 - zilbuz:issue-44837, r=nikomatsakis
MIR-borrowck: add permisson checks to `fn access_lvalue` WIP : Some FIXME left and some broken tests. Fix #44837
2 parents e21df80 + cbad2e5 commit 212d74f

File tree

5 files changed

+260
-25
lines changed

5 files changed

+260
-25
lines changed

‎src/librustc/ty/util.rs

+22-1
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,9 @@
1010

1111
//! misc. type-system utilities too small to deserve their own file
1212
13+
use hir::def::Def;
1314
use hir::def_id::{DefId, LOCAL_CRATE};
14-
use hir::map::DefPathData;
15+
use hir::map::{DefPathData, Node};
1516
use hir;
1617
use ich::NodeIdHashingMode;
1718
use middle::const_val::ConstVal;
@@ -648,6 +649,26 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
648649
_ => bug!(),
649650
}
650651
}
652+
653+
/// Check if the node pointed to by def_id is a mutable static item
654+
pub fn is_static_mut(&self, def_id: DefId) -> bool {
655+
if let Some(node) = self.hir.get_if_local(def_id) {
656+
match node {
657+
Node::NodeItem(&hir::Item {
658+
node: hir::ItemStatic(_, hir::MutMutable, _), ..
659+
}) => true,
660+
Node::NodeForeignItem(&hir::ForeignItem {
661+
node: hir::ForeignItemStatic(_, mutbl), ..
662+
}) => mutbl,
663+
_ => false
664+
}
665+
} else {
666+
match self.describe_def(def_id) {
667+
Some(Def::Static(_, mutbl)) => mutbl,
668+
_ => false
669+
}
670+
}
671+
}
651672
}
652673

653674
pub struct TypeIdHasher<'a, 'gcx: 'a+'tcx, 'tcx: 'a, W> {

‎src/librustc_mir/borrow_check.rs

+154-2
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
//! This query borrow-checks the MIR to (further) ensure it is not broken.
1212
13+
use rustc::hir;
1314
use rustc::hir::def_id::{DefId};
1415
use rustc::infer::{InferCtxt};
1516
use rustc::ty::{self, TyCtxt, ParamEnv};
@@ -447,9 +448,12 @@ impl<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a> MirBorrowckCtxt<'c, 'b, 'a, 'gcx, 'tcx>
447448
lvalue_span: (&Lvalue<'tcx>, Span),
448449
kind: (ShallowOrDeep, ReadOrWrite),
449450
flow_state: &InProgress<'b, 'gcx, 'tcx>) {
450-
// FIXME: also need to check permissions (e.g. reject mut
451-
// borrow of immutable ref, moves through non-`Box`-ref)
451+
452452
let (sd, rw) = kind;
453+
454+
// Check permissions
455+
self.check_access_permissions(lvalue_span, rw);
456+
453457
self.each_borrow_involving_path(
454458
context, (sd, lvalue_span.0), flow_state, |this, _index, borrow, common_prefix| {
455459
match (rw, borrow.kind) {
@@ -861,6 +865,154 @@ impl<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a> MirBorrowckCtxt<'c, 'b, 'a, 'gcx, 'tcx>
861865
}
862866
}
863867
}
868+
869+
/// Check the permissions for the given lvalue and read or write kind
870+
fn check_access_permissions(&self, (lvalue, span): (&Lvalue<'tcx>, Span), kind: ReadOrWrite) {
871+
match kind {
872+
Write(WriteKind::MutableBorrow(BorrowKind::Unique)) => {
873+
if let Err(_lvalue_err) = self.is_unique(lvalue) {
874+
span_bug!(span, "&unique borrow for `{}` should not fail",
875+
self.describe_lvalue(lvalue));
876+
}
877+
},
878+
Write(WriteKind::MutableBorrow(BorrowKind::Mut)) => {
879+
if let Err(lvalue_err) = self.is_mutable(lvalue) {
880+
let mut err = self.tcx.cannot_borrow_path_as_mutable(span,
881+
&format!("immutable item `{}`",
882+
self.describe_lvalue(lvalue)),
883+
Origin::Mir);
884+
err.span_label(span, "cannot borrow as mutable");
885+
886+
if lvalue != lvalue_err {
887+
err.note(&format!("Value not mutable causing this error: `{}`",
888+
self.describe_lvalue(lvalue_err)));
889+
}
890+
891+
err.emit();
892+
}
893+
},
894+
_ => {}// Access authorized
895+
}
896+
}
897+
898+
/// Can this value be written or borrowed mutably
899+
fn is_mutable<'d>(&self, lvalue: &'d Lvalue<'tcx>) -> Result<(), &'d Lvalue<'tcx>> {
900+
match *lvalue {
901+
Lvalue::Local(local) => {
902+
let local = &self.mir.local_decls[local];
903+
match local.mutability {
904+
Mutability::Not => Err(lvalue),
905+
Mutability::Mut => Ok(())
906+
}
907+
},
908+
Lvalue::Static(ref static_) => {
909+
if !self.tcx.is_static_mut(static_.def_id) {
910+
Err(lvalue)
911+
} else {
912+
Ok(())
913+
}
914+
},
915+
Lvalue::Projection(ref proj) => {
916+
match proj.elem {
917+
ProjectionElem::Deref => {
918+
let base_ty = proj.base.ty(self.mir, self.tcx).to_ty(self.tcx);
919+
920+
// `Box<T>` owns its content, so mutable if its location is mutable
921+
if base_ty.is_box() {
922+
return self.is_mutable(&proj.base);
923+
}
924+
925+
// Otherwise we check the kind of deref to decide
926+
match base_ty.sty {
927+
ty::TyRef(_, tnm) => {
928+
match tnm.mutbl {
929+
// Shared borrowed data is never mutable
930+
hir::MutImmutable => Err(lvalue),
931+
// Mutably borrowed data is mutable, but only if we have a
932+
// unique path to the `&mut`
933+
hir::MutMutable => self.is_unique(&proj.base),
934+
}
935+
},
936+
ty::TyRawPtr(tnm) => {
937+
match tnm.mutbl {
938+
// `*const` raw pointers are not mutable
939+
hir::MutImmutable => Err(lvalue),
940+
// `*mut` raw pointers are always mutable, regardless of context
941+
// The users have to check by themselve.
942+
hir::MutMutable => Ok(()),
943+
}
944+
},
945+
// Deref should only be for reference, pointers or boxes
946+
_ => bug!("Deref of unexpected type: {:?}", base_ty)
947+
}
948+
},
949+
// All other projections are owned by their base path, so mutable if
950+
// base path is mutable
951+
ProjectionElem::Field(..) |
952+
ProjectionElem::Index(..) |
953+
ProjectionElem::ConstantIndex{..} |
954+
ProjectionElem::Subslice{..} |
955+
ProjectionElem::Downcast(..) =>
956+
self.is_mutable(&proj.base)
957+
}
958+
}
959+
}
960+
}
961+
962+
/// Does this lvalue have a unique path
963+
fn is_unique<'d>(&self, lvalue: &'d Lvalue<'tcx>) -> Result<(), &'d Lvalue<'tcx>> {
964+
match *lvalue {
965+
Lvalue::Local(..) => {
966+
// Local variables are unique
967+
Ok(())
968+
},
969+
Lvalue::Static(..) => {
970+
// Static variables are not
971+
Err(lvalue)
972+
},
973+
Lvalue::Projection(ref proj) => {
974+
match proj.elem {
975+
ProjectionElem::Deref => {
976+
let base_ty = proj.base.ty(self.mir, self.tcx).to_ty(self.tcx);
977+
978+
// `Box<T>` referent is unique if box is a unique spot
979+
if base_ty.is_box() {
980+
return self.is_unique(&proj.base);
981+
}
982+
983+
// Otherwise we check the kind of deref to decide
984+
match base_ty.sty {
985+
ty::TyRef(_, tnm) => {
986+
match tnm.mutbl {
987+
// lvalue represent an aliased location
988+
hir::MutImmutable => Err(lvalue),
989+
// `&mut T` is as unique as the context in which it is found
990+
hir::MutMutable => self.is_unique(&proj.base),
991+
}
992+
},
993+
ty::TyRawPtr(tnm) => {
994+
match tnm.mutbl {
995+
// `*mut` can be aliased, but we leave it to user
996+
hir::MutMutable => Ok(()),
997+
// `*const` is treated the same as `*mut`
998+
hir::MutImmutable => Ok(()),
999+
}
1000+
},
1001+
// Deref should only be for reference, pointers or boxes
1002+
_ => bug!("Deref of unexpected type: {:?}", base_ty)
1003+
}
1004+
},
1005+
// Other projections are unique if the base is unique
1006+
ProjectionElem::Field(..) |
1007+
ProjectionElem::Index(..) |
1008+
ProjectionElem::ConstantIndex{..} |
1009+
ProjectionElem::Subslice{..} |
1010+
ProjectionElem::Downcast(..) =>
1011+
self.is_unique(&proj.base)
1012+
}
1013+
}
1014+
}
1015+
}
8641016
}
8651017

8661018
#[derive(Copy, Clone, PartialEq, Eq, Debug)]

‎src/librustc_mir/transform/check_unsafety.rs

+2-21
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,8 @@ use rustc_data_structures::indexed_vec::IndexVec;
1414
use rustc::ty::maps::Providers;
1515
use rustc::ty::{self, TyCtxt};
1616
use rustc::hir;
17-
use rustc::hir::def::Def;
1817
use rustc::hir::def_id::DefId;
19-
use rustc::hir::map::{DefPathData, Node};
18+
use rustc::hir::map::DefPathData;
2019
use rustc::lint::builtin::{SAFE_EXTERN_STATICS, UNUSED_UNSAFE};
2120
use rustc::mir::*;
2221
use rustc::mir::visit::{LvalueContext, Visitor};
@@ -189,7 +188,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
189188
// locals are safe
190189
}
191190
&Lvalue::Static(box Static { def_id, ty: _ }) => {
192-
if self.is_static_mut(def_id) {
191+
if self.tcx.is_static_mut(def_id) {
193192
self.require_unsafe("use of mutable static");
194193
} else if self.tcx.is_foreign_item(def_id) {
195194
let source_info = self.source_info;
@@ -208,24 +207,6 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
208207
}
209208

210209
impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
211-
fn is_static_mut(&self, def_id: DefId) -> bool {
212-
if let Some(node) = self.tcx.hir.get_if_local(def_id) {
213-
match node {
214-
Node::NodeItem(&hir::Item {
215-
node: hir::ItemStatic(_, hir::MutMutable, _), ..
216-
}) => true,
217-
Node::NodeForeignItem(&hir::ForeignItem {
218-
node: hir::ForeignItemStatic(_, mutbl), ..
219-
}) => mutbl,
220-
_ => false
221-
}
222-
} else {
223-
match self.tcx.describe_def(def_id) {
224-
Some(Def::Static(_, mutbl)) => mutbl,
225-
_ => false
226-
}
227-
}
228-
}
229210
fn require_unsafe(&mut self,
230211
description: &'static str)
231212
{

‎src/test/compile-fail/E0596.rs

+6-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,12 @@
88
// option. This file may not be copied, modified, or distributed
99
// except according to those terms.
1010

11+
// revisions: ast mir
12+
//[mir]compile-flags: -Z emit-end-regions -Z borrowck-mir
13+
1114
fn main() {
1215
let x = 1;
13-
let y = &mut x; //~ ERROR E0596
16+
let y = &mut x; //[ast]~ ERROR [E0596]
17+
//[mir]~^ ERROR (Ast) [E0596]
18+
//[mir]~| ERROR (Mir) [E0596]
1419
}

‎src/test/compile-fail/borrowck/borrowck-access-permissions.rs

+76
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// revisions: ast mir
12+
//[mir]compile-flags: -Z emit-end-regions -Z borrowck-mir
13+
14+
static static_x : i32 = 1;
15+
static mut static_x_mut : i32 = 1;
16+
17+
fn main() {
18+
let x = 1;
19+
let mut x_mut = 1;
20+
21+
{ // borrow of local
22+
let _y1 = &mut x; //[ast]~ ERROR [E0596]
23+
//[mir]~^ ERROR (Ast) [E0596]
24+
//[mir]~| ERROR (Mir) [E0596]
25+
let _y2 = &mut x_mut; // No error
26+
}
27+
28+
{ // borrow of static
29+
let _y1 = &mut static_x; //[ast]~ ERROR [E0596]
30+
//[mir]~^ ERROR (Ast) [E0596]
31+
//[mir]~| ERROR (Mir) [E0596]
32+
unsafe { let _y2 = &mut static_x_mut; } // No error
33+
}
34+
35+
{ // borrow of deref to box
36+
let box_x = Box::new(1);
37+
let mut box_x_mut = Box::new(1);
38+
39+
let _y1 = &mut *box_x; //[ast]~ ERROR [E0596]
40+
//[mir]~^ ERROR (Ast) [E0596]
41+
//[mir]~| ERROR (Mir) [E0596]
42+
let _y2 = &mut *box_x_mut; // No error
43+
}
44+
45+
{ // borrow of deref to reference
46+
let ref_x = &x;
47+
let ref_x_mut = &mut x_mut;
48+
49+
let _y1 = &mut *ref_x; //[ast]~ ERROR [E0596]
50+
//[mir]~^ ERROR (Ast) [E0596]
51+
//[mir]~| ERROR (Mir) [E0596]
52+
let _y2 = &mut *ref_x_mut; // No error
53+
}
54+
55+
{ // borrow of deref to pointer
56+
let ptr_x : *const _ = &x;
57+
let ptr_mut_x : *mut _ = &mut x_mut;
58+
59+
unsafe {
60+
let _y1 = &mut *ptr_x; //[ast]~ ERROR [E0596]
61+
//[mir]~^ ERROR (Ast) [E0596]
62+
//[mir]~| ERROR (Mir) [E0596]
63+
let _y2 = &mut *ptr_mut_x; // No error
64+
}
65+
}
66+
67+
{ // borrowing mutably through an immutable reference
68+
struct Foo<'a> { f: &'a mut i32, g: &'a i32 };
69+
let mut foo = Foo { f: &mut x_mut, g: &x };
70+
let foo_ref = &foo;
71+
let _y = &mut *foo_ref.f; //[ast]~ ERROR [E0389]
72+
//[mir]~^ ERROR (Ast) [E0389]
73+
//[mir]~| ERROR (Mir) [E0596]
74+
// FIXME: Wrong error in MIR
75+
}
76+
}

0 commit comments

Comments
 (0)