From 0bb77bdb5423402b583372abf2f097be6b4e46bc Mon Sep 17 00:00:00 2001 From: Basile Desloges Date: Sun, 12 Nov 2017 12:40:56 +0100 Subject: [PATCH 1/3] mir-borrowck: Move `is_static_mut()` to `ty/utils.rs` --- src/librustc/ty/util.rs | 23 +++++++++++++++++++- src/librustc_mir/transform/check_unsafety.rs | 23 ++------------------ 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/librustc/ty/util.rs b/src/librustc/ty/util.rs index d12a973017d..aa07a6b070e 100644 --- a/src/librustc/ty/util.rs +++ b/src/librustc/ty/util.rs @@ -10,8 +10,9 @@ //! misc. type-system utilities too small to deserve their own file +use hir::def::Def; use hir::def_id::{DefId, LOCAL_CRATE}; -use hir::map::DefPathData; +use hir::map::{DefPathData, Node}; use hir; use ich::NodeIdHashingMode; use middle::const_val::ConstVal; @@ -648,6 +649,26 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { _ => bug!(), } } + + /// Check if the node pointed to by def_id is a mutable static item + pub fn is_static_mut(&self, def_id: DefId) -> bool { + if let Some(node) = self.hir.get_if_local(def_id) { + match node { + Node::NodeItem(&hir::Item { + node: hir::ItemStatic(_, hir::MutMutable, _), .. + }) => true, + Node::NodeForeignItem(&hir::ForeignItem { + node: hir::ForeignItemStatic(_, mutbl), .. + }) => mutbl, + _ => false + } + } else { + match self.describe_def(def_id) { + Some(Def::Static(_, mutbl)) => mutbl, + _ => false + } + } + } } pub struct TypeIdHasher<'a, 'gcx: 'a+'tcx, 'tcx: 'a, W> { diff --git a/src/librustc_mir/transform/check_unsafety.rs b/src/librustc_mir/transform/check_unsafety.rs index 53a4b2551fd..cbf1b0b0899 100644 --- a/src/librustc_mir/transform/check_unsafety.rs +++ b/src/librustc_mir/transform/check_unsafety.rs @@ -14,9 +14,8 @@ use rustc_data_structures::indexed_vec::IndexVec; use rustc::ty::maps::Providers; use rustc::ty::{self, TyCtxt}; use rustc::hir; -use rustc::hir::def::Def; use rustc::hir::def_id::DefId; -use rustc::hir::map::{DefPathData, Node}; +use rustc::hir::map::DefPathData; use rustc::lint::builtin::{SAFE_EXTERN_STATICS, UNUSED_UNSAFE}; use rustc::mir::*; use rustc::mir::visit::{LvalueContext, Visitor}; @@ -189,7 +188,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { // locals are safe } &Lvalue::Static(box Static { def_id, ty: _ }) => { - if self.is_static_mut(def_id) { + if self.tcx.is_static_mut(def_id) { self.require_unsafe("use of mutable static"); } else if self.tcx.is_foreign_item(def_id) { let source_info = self.source_info; @@ -208,24 +207,6 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { } impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> { - fn is_static_mut(&self, def_id: DefId) -> bool { - if let Some(node) = self.tcx.hir.get_if_local(def_id) { - match node { - Node::NodeItem(&hir::Item { - node: hir::ItemStatic(_, hir::MutMutable, _), .. - }) => true, - Node::NodeForeignItem(&hir::ForeignItem { - node: hir::ForeignItemStatic(_, mutbl), .. - }) => mutbl, - _ => false - } - } else { - match self.tcx.describe_def(def_id) { - Some(Def::Static(_, mutbl)) => mutbl, - _ => false - } - } - } fn require_unsafe(&mut self, description: &'static str) { From 37e08a5dcab37aa3120eccadf7e725d0008110ec Mon Sep 17 00:00:00 2001 From: Basile Desloges Date: Sat, 21 Oct 2017 21:15:04 +0200 Subject: [PATCH 2/3] mir-borrowck: Check access permissions in `access_lvalue()` --- src/librustc_mir/borrow_check.rs | 156 ++++++++++++++++++++++++++++++- 1 file changed, 154 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/borrow_check.rs b/src/librustc_mir/borrow_check.rs index b723b86776b..c56631d429c 100644 --- a/src/librustc_mir/borrow_check.rs +++ b/src/librustc_mir/borrow_check.rs @@ -10,6 +10,7 @@ //! This query borrow-checks the MIR to (further) ensure it is not broken. +use rustc::hir; use rustc::hir::def_id::{DefId}; use rustc::infer::{InferCtxt}; 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> lvalue_span: (&Lvalue<'tcx>, Span), kind: (ShallowOrDeep, ReadOrWrite), flow_state: &InProgress<'b, 'gcx, 'tcx>) { - // FIXME: also need to check permissions (e.g. reject mut - // borrow of immutable ref, moves through non-`Box`-ref) + let (sd, rw) = kind; + + // Check permissions + self.check_access_permissions(lvalue_span, rw); + self.each_borrow_involving_path( context, (sd, lvalue_span.0), flow_state, |this, _index, borrow, common_prefix| { match (rw, borrow.kind) { @@ -861,6 +865,154 @@ impl<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a> MirBorrowckCtxt<'c, 'b, 'a, 'gcx, 'tcx> } } } + + /// Check the permissions for the given lvalue and read or write kind + fn check_access_permissions(&self, (lvalue, span): (&Lvalue<'tcx>, Span), kind: ReadOrWrite) { + match kind { + Write(WriteKind::MutableBorrow(BorrowKind::Unique)) => { + if let Err(_lvalue_err) = self.is_unique(lvalue) { + span_bug!(span, "&unique borrow for `{}` should not fail", + self.describe_lvalue(lvalue)); + } + }, + Write(WriteKind::MutableBorrow(BorrowKind::Mut)) => { + if let Err(lvalue_err) = self.is_mutable(lvalue) { + let mut err = self.tcx.cannot_borrow_path_as_mutable(span, + &format!("immutable item `{}`", + self.describe_lvalue(lvalue)), + Origin::Mir); + err.span_label(span, "cannot borrow as mutable"); + + if lvalue != lvalue_err { + err.note(&format!("Value not mutable causing this error: `{}`", + self.describe_lvalue(lvalue_err))); + } + + err.emit(); + } + }, + _ => {}// Access authorized + } + } + + /// Can this value be written or borrowed mutably + fn is_mutable<'d>(&self, lvalue: &'d Lvalue<'tcx>) -> Result<(), &'d Lvalue<'tcx>> { + match *lvalue { + Lvalue::Local(local) => { + let local = &self.mir.local_decls[local]; + match local.mutability { + Mutability::Not => Err(lvalue), + Mutability::Mut => Ok(()) + } + }, + Lvalue::Static(ref static_) => { + if !self.tcx.is_static_mut(static_.def_id) { + Err(lvalue) + } else { + Ok(()) + } + }, + Lvalue::Projection(ref proj) => { + match proj.elem { + ProjectionElem::Deref => { + let base_ty = proj.base.ty(self.mir, self.tcx).to_ty(self.tcx); + + // `Box` owns its content, so mutable if its location is mutable + if base_ty.is_box() { + return self.is_mutable(&proj.base); + } + + // Otherwise we check the kind of deref to decide + match base_ty.sty { + ty::TyRef(_, tnm) => { + match tnm.mutbl { + // Shared borrowed data is never mutable + hir::MutImmutable => Err(lvalue), + // Mutably borrowed data is mutable, but only if we have a + // unique path to the `&mut` + hir::MutMutable => self.is_unique(&proj.base), + } + }, + ty::TyRawPtr(tnm) => { + match tnm.mutbl { + // `*const` raw pointers are not mutable + hir::MutImmutable => Err(lvalue), + // `*mut` raw pointers are always mutable, regardless of context + // The users have to check by themselve. + hir::MutMutable => Ok(()), + } + }, + // Deref should only be for reference, pointers or boxes + _ => bug!("Deref of unexpected type: {:?}", base_ty) + } + }, + // All other projections are owned by their base path, so mutable if + // base path is mutable + ProjectionElem::Field(..) | + ProjectionElem::Index(..) | + ProjectionElem::ConstantIndex{..} | + ProjectionElem::Subslice{..} | + ProjectionElem::Downcast(..) => + self.is_mutable(&proj.base) + } + } + } + } + + /// Does this lvalue have a unique path + fn is_unique<'d>(&self, lvalue: &'d Lvalue<'tcx>) -> Result<(), &'d Lvalue<'tcx>> { + match *lvalue { + Lvalue::Local(..) => { + // Local variables are unique + Ok(()) + }, + Lvalue::Static(..) => { + // Static variables are not + Err(lvalue) + }, + Lvalue::Projection(ref proj) => { + match proj.elem { + ProjectionElem::Deref => { + let base_ty = proj.base.ty(self.mir, self.tcx).to_ty(self.tcx); + + // `Box` referent is unique if box is a unique spot + if base_ty.is_box() { + return self.is_unique(&proj.base); + } + + // Otherwise we check the kind of deref to decide + match base_ty.sty { + ty::TyRef(_, tnm) => { + match tnm.mutbl { + // lvalue represent an aliased location + hir::MutImmutable => Err(lvalue), + // `&mut T` is as unique as the context in which it is found + hir::MutMutable => self.is_unique(&proj.base), + } + }, + ty::TyRawPtr(tnm) => { + match tnm.mutbl { + // `*mut` can be aliased, but we leave it to user + hir::MutMutable => Ok(()), + // `*const` is treated the same as `*mut` + hir::MutImmutable => Ok(()), + } + }, + // Deref should only be for reference, pointers or boxes + _ => bug!("Deref of unexpected type: {:?}", base_ty) + } + }, + // Other projections are unique if the base is unique + ProjectionElem::Field(..) | + ProjectionElem::Index(..) | + ProjectionElem::ConstantIndex{..} | + ProjectionElem::Subslice{..} | + ProjectionElem::Downcast(..) => + self.is_unique(&proj.base) + } + } + } + } } #[derive(Copy, Clone, PartialEq, Eq, Debug)] From cbad2e5720fd251fcd7cbe9648fd7671373a71e7 Mon Sep 17 00:00:00 2001 From: Basile Desloges Date: Sat, 21 Oct 2017 21:15:28 +0200 Subject: [PATCH 3/3] mir-borrowck: Test for `check_access_permissions()` --- src/test/compile-fail/E0596.rs | 7 +- .../borrowck/borrowck-access-permissions.rs | 76 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 src/test/compile-fail/borrowck/borrowck-access-permissions.rs diff --git a/src/test/compile-fail/E0596.rs b/src/test/compile-fail/E0596.rs index 1f1af69d977..0366d4d134a 100644 --- a/src/test/compile-fail/E0596.rs +++ b/src/test/compile-fail/E0596.rs @@ -8,7 +8,12 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// revisions: ast mir +//[mir]compile-flags: -Z emit-end-regions -Z borrowck-mir + fn main() { let x = 1; - let y = &mut x; //~ ERROR E0596 + let y = &mut x; //[ast]~ ERROR [E0596] + //[mir]~^ ERROR (Ast) [E0596] + //[mir]~| ERROR (Mir) [E0596] } diff --git a/src/test/compile-fail/borrowck/borrowck-access-permissions.rs b/src/test/compile-fail/borrowck/borrowck-access-permissions.rs new file mode 100644 index 00000000000..fbe219102ed --- /dev/null +++ b/src/test/compile-fail/borrowck/borrowck-access-permissions.rs @@ -0,0 +1,76 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// revisions: ast mir +//[mir]compile-flags: -Z emit-end-regions -Z borrowck-mir + +static static_x : i32 = 1; +static mut static_x_mut : i32 = 1; + +fn main() { + let x = 1; + let mut x_mut = 1; + + { // borrow of local + let _y1 = &mut x; //[ast]~ ERROR [E0596] + //[mir]~^ ERROR (Ast) [E0596] + //[mir]~| ERROR (Mir) [E0596] + let _y2 = &mut x_mut; // No error + } + + { // borrow of static + let _y1 = &mut static_x; //[ast]~ ERROR [E0596] + //[mir]~^ ERROR (Ast) [E0596] + //[mir]~| ERROR (Mir) [E0596] + unsafe { let _y2 = &mut static_x_mut; } // No error + } + + { // borrow of deref to box + let box_x = Box::new(1); + let mut box_x_mut = Box::new(1); + + let _y1 = &mut *box_x; //[ast]~ ERROR [E0596] + //[mir]~^ ERROR (Ast) [E0596] + //[mir]~| ERROR (Mir) [E0596] + let _y2 = &mut *box_x_mut; // No error + } + + { // borrow of deref to reference + let ref_x = &x; + let ref_x_mut = &mut x_mut; + + let _y1 = &mut *ref_x; //[ast]~ ERROR [E0596] + //[mir]~^ ERROR (Ast) [E0596] + //[mir]~| ERROR (Mir) [E0596] + let _y2 = &mut *ref_x_mut; // No error + } + + { // borrow of deref to pointer + let ptr_x : *const _ = &x; + let ptr_mut_x : *mut _ = &mut x_mut; + + unsafe { + let _y1 = &mut *ptr_x; //[ast]~ ERROR [E0596] + //[mir]~^ ERROR (Ast) [E0596] + //[mir]~| ERROR (Mir) [E0596] + let _y2 = &mut *ptr_mut_x; // No error + } + } + + { // borrowing mutably through an immutable reference + struct Foo<'a> { f: &'a mut i32, g: &'a i32 }; + let mut foo = Foo { f: &mut x_mut, g: &x }; + let foo_ref = &foo; + let _y = &mut *foo_ref.f; //[ast]~ ERROR [E0389] + //[mir]~^ ERROR (Ast) [E0389] + //[mir]~| ERROR (Mir) [E0596] + // FIXME: Wrong error in MIR + } +}