From 2ff89c5614a381849ec4f4b2d06b5eb274850371 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sun, 26 May 2019 10:00:34 +0100 Subject: [PATCH 1/3] Avoid a string comparison in MIR construction --- src/librustc_mir/build/expr/into.rs | 5 ++--- src/libsyntax_pos/symbol.rs | 1 + 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/build/expr/into.rs b/src/librustc_mir/build/expr/into.rs index e7388b920548b..c9e5fca4d75e7 100644 --- a/src/librustc_mir/build/expr/into.rs +++ b/src/librustc_mir/build/expr/into.rs @@ -200,16 +200,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ty::FnDef(def_id, _) => { let f = ty.fn_sig(this.hir.tcx()); if f.abi() == Abi::RustIntrinsic || f.abi() == Abi::PlatformIntrinsic { - Some(this.hir.tcx().item_name(def_id).as_str()) + Some(this.hir.tcx().item_name(def_id)) } else { None } } _ => None, }; - let intrinsic = intrinsic.as_ref().map(|s| &s[..]); let fun = unpack!(block = this.as_local_operand(block, fun)); - if intrinsic == Some("move_val_init") { + if let Some(sym::move_val_init) = intrinsic { // `move_val_init` has "magic" semantics - the second argument is // always evaluated "directly" into the first one. diff --git a/src/libsyntax_pos/symbol.rs b/src/libsyntax_pos/symbol.rs index c41f413970ff5..b3e9576f43f59 100644 --- a/src/libsyntax_pos/symbol.rs +++ b/src/libsyntax_pos/symbol.rs @@ -432,6 +432,7 @@ symbols! { module, module_path, more_struct_aliases, + move_val_init, movbe_target_feature, must_use, naked, From 732081829263fd02b6995ff815d4c001cce460cf Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sat, 19 Oct 2019 21:00:21 +0100 Subject: [PATCH 2/3] Fix soundness issue with index bounds checks An expression like `x[1][{ x = y; 2}]` would perform the bounds check for the inner index operation before evaluating the outer index. This would allow out of bounds memory accesses. --- src/librustc/mir/mod.rs | 14 +- .../borrow_check/conflict_errors.rs | 154 ++++++--- src/librustc_mir/build/expr/as_place.rs | 303 +++++++++++++++--- src/librustc_mir/util/borrowck_errors.rs | 14 +- .../mir-opt/storage_live_dead_in_statics.rs | 4 +- .../mir-opt/uninhabited_enum_branching.rs | 18 +- .../slice-index-bounds-check-invalidation.rs | 82 +++++ ...ice-index-bounds-check-invalidation.stderr | 35 ++ 8 files changed, 500 insertions(+), 124 deletions(-) create mode 100644 src/test/ui/borrowck/slice-index-bounds-check-invalidation.rs create mode 100644 src/test/ui/borrowck/slice-index-bounds-check-invalidation.stderr diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index a3ddfec765f3f..fd2063e2da984 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -1665,6 +1665,15 @@ pub enum FakeReadCause { /// Therefore, we insert a "fake read" here to ensure that we get /// appropriate errors. ForLet, + + /// If we have an index expression like + /// + /// (*x)[1][{ x = y; 4}] + /// + /// then the first bounds check is invalidated when we evaluate the second + /// index expression. Thus we create a fake borrow of `x` across the second + /// indexer, which will cause a borrow check error. + ForIndex, } #[derive(Clone, Debug, RustcEncodable, RustcDecodable, HashStable)] @@ -1764,9 +1773,8 @@ impl_stable_hash_for!(struct Static<'tcx> { def_id }); -#[derive( - Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable, HashStable, -)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(RustcEncodable, RustcDecodable, HashStable)] pub enum ProjectionElem { Deref, Field(Field, T), diff --git a/src/librustc_mir/borrow_check/conflict_errors.rs b/src/librustc_mir/borrow_check/conflict_errors.rs index 0913d743328a7..ebc25138a0619 100644 --- a/src/librustc_mir/borrow_check/conflict_errors.rs +++ b/src/librustc_mir/borrow_check/conflict_errors.rs @@ -2,9 +2,9 @@ use rustc::hir; use rustc::hir::def_id::DefId; use rustc::hir::{AsyncGeneratorKind, GeneratorKind}; use rustc::mir::{ - self, AggregateKind, BindingForm, BorrowKind, ClearCrossCrate, ConstraintCategory, Local, - LocalDecl, LocalKind, Location, Operand, Place, PlaceBase, PlaceRef, ProjectionElem, Rvalue, - Statement, StatementKind, TerminatorKind, VarBindingForm, + self, AggregateKind, BindingForm, BorrowKind, ClearCrossCrate, ConstraintCategory, + FakeReadCause, Local, LocalDecl, LocalKind, Location, Operand, Place, PlaceBase, PlaceRef, + ProjectionElem, Rvalue, Statement, StatementKind, TerminatorKind, VarBindingForm, }; use rustc::ty::{self, Ty}; use rustc_data_structures::fx::FxHashSet; @@ -380,42 +380,38 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { let first_borrow_desc; let mut err = match ( gen_borrow_kind, - "immutable", - "mutable", issued_borrow.kind, - "immutable", - "mutable", ) { - (BorrowKind::Shared, lft, _, BorrowKind::Mut { .. }, _, rgt) => { + (BorrowKind::Shared, BorrowKind::Mut { .. }) => { first_borrow_desc = "mutable "; self.cannot_reborrow_already_borrowed( span, &desc_place, &msg_place, - lft, + "immutable", issued_span, "it", - rgt, + "mutable", &msg_borrow, None, ) } - (BorrowKind::Mut { .. }, _, lft, BorrowKind::Shared, rgt, _) => { + (BorrowKind::Mut { .. }, BorrowKind::Shared) => { first_borrow_desc = "immutable "; self.cannot_reborrow_already_borrowed( span, &desc_place, &msg_place, - lft, + "mutable", issued_span, "it", - rgt, + "immutable", &msg_borrow, None, ) } - (BorrowKind::Mut { .. }, _, _, BorrowKind::Mut { .. }, _, _) => { + (BorrowKind::Mut { .. }, BorrowKind::Mut { .. }) => { first_borrow_desc = "first "; self.cannot_mutably_borrow_multiply( span, @@ -427,7 +423,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { ) } - (BorrowKind::Unique, _, _, BorrowKind::Unique, _, _) => { + (BorrowKind::Unique, BorrowKind::Unique) => { first_borrow_desc = "first "; self.cannot_uniquely_borrow_by_two_closures( span, @@ -437,25 +433,45 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { ) } - (BorrowKind::Mut { .. }, _, _, BorrowKind::Shallow, _, _) - | (BorrowKind::Unique, _, _, BorrowKind::Shallow, _, _) => { - let mut err = self.cannot_mutate_in_match_guard( - span, - issued_span, - &desc_place, - "mutably borrow", - ); - borrow_spans.var_span_label( - &mut err, - format!( - "borrow occurs due to use of `{}`{}", desc_place, borrow_spans.describe() - ), - ); + (BorrowKind::Mut { .. }, BorrowKind::Shallow) + | (BorrowKind::Unique, BorrowKind::Shallow) => { + if let Some(immutable_section_description) = self.classify_immutable_section( + &issued_borrow.assigned_place, + ) { + let mut err = self.cannot_mutate_in_immutable_section( + span, + issued_span, + &desc_place, + immutable_section_description, + "mutably borrow", + ); + borrow_spans.var_span_label( + &mut err, + format!( + "borrow occurs due to use of `{}`{}", + desc_place, + borrow_spans.describe(), + ), + ); - return err; + return err; + } else { + first_borrow_desc = "immutable "; + self.cannot_reborrow_already_borrowed( + span, + &desc_place, + &msg_place, + "mutable", + issued_span, + "it", + "immutable", + &msg_borrow, + None, + ) + } } - (BorrowKind::Unique, _, _, _, _, _) => { + (BorrowKind::Unique, _) => { first_borrow_desc = "first "; self.cannot_uniquely_borrow_by_one_closure( span, @@ -469,14 +485,14 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { ) }, - (BorrowKind::Shared, lft, _, BorrowKind::Unique, _, _) => { + (BorrowKind::Shared, BorrowKind::Unique) => { first_borrow_desc = "first "; self.cannot_reborrow_already_uniquely_borrowed( span, container_name, &desc_place, "", - lft, + "immutable", issued_span, "", None, @@ -484,14 +500,14 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { ) } - (BorrowKind::Mut { .. }, _, lft, BorrowKind::Unique, _, _) => { + (BorrowKind::Mut { .. }, BorrowKind::Unique) => { first_borrow_desc = "first "; self.cannot_reborrow_already_uniquely_borrowed( span, container_name, &desc_place, "", - lft, + "mutable", issued_span, "", None, @@ -499,12 +515,12 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { ) } - (BorrowKind::Shared, _, _, BorrowKind::Shared, _, _) - | (BorrowKind::Shared, _, _, BorrowKind::Shallow, _, _) - | (BorrowKind::Shallow, _, _, BorrowKind::Mut { .. }, _, _) - | (BorrowKind::Shallow, _, _, BorrowKind::Unique, _, _) - | (BorrowKind::Shallow, _, _, BorrowKind::Shared, _, _) - | (BorrowKind::Shallow, _, _, BorrowKind::Shallow, _, _) => unreachable!(), + (BorrowKind::Shared, BorrowKind::Shared) + | (BorrowKind::Shared, BorrowKind::Shallow) + | (BorrowKind::Shallow, BorrowKind::Mut { .. }) + | (BorrowKind::Shallow, BorrowKind::Unique) + | (BorrowKind::Shallow, BorrowKind::Shared) + | (BorrowKind::Shallow, BorrowKind::Shallow) => unreachable!(), }; if issued_spans == borrow_spans { @@ -1429,20 +1445,23 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { let loan_span = loan_spans.args_or_use(); if loan.kind == BorrowKind::Shallow { - let mut err = self.cannot_mutate_in_match_guard( - span, - loan_span, - &self.describe_place(place.as_ref()).unwrap_or_else(|| "_".to_owned()), - "assign", - ); - loan_spans.var_span_label( - &mut err, - format!("borrow occurs due to use{}", loan_spans.describe()), - ); + if let Some(section) = self.classify_immutable_section(&loan.assigned_place) { + let mut err = self.cannot_mutate_in_immutable_section( + span, + loan_span, + &self.describe_place(place.as_ref()).unwrap_or_else(|| "_".to_owned()), + section, + "assign", + ); + loan_spans.var_span_label( + &mut err, + format!("borrow occurs due to use{}", loan_spans.describe()), + ); - err.buffer(&mut self.errors_buffer); + err.buffer(&mut self.errors_buffer); - return; + return; + } } let mut err = self.cannot_assign_to_borrowed( @@ -1593,6 +1612,35 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { } } + /// Describe the reason for the fake borrow that was assigned to `place`. + fn classify_immutable_section(&self, place: &Place<'tcx>) -> Option<&'static str> { + use rustc::mir::visit::Visitor; + struct FakeReadCauseFinder<'a, 'tcx> { + place: &'a Place<'tcx>, + cause: Option, + } + impl<'tcx> Visitor<'tcx> for FakeReadCauseFinder<'_, 'tcx> { + fn visit_statement(&mut self, statement: &Statement<'tcx>, _: Location) { + match statement { + Statement { + kind: StatementKind::FakeRead(cause, box ref place), + .. + } if *place == *self.place => { + self.cause = Some(*cause); + } + _ => (), + } + } + } + let mut visitor = FakeReadCauseFinder { place, cause: None }; + visitor.visit_body(&self.body); + match visitor.cause { + Some(FakeReadCause::ForMatchGuard) => Some("match guard"), + Some(FakeReadCause::ForIndex) => Some("indexing expression"), + _ => None, + } + } + /// Annotate argument and return type of function and closure with (synthesized) lifetime for /// borrow of local value that does not live long enough. fn annotate_argument_and_return_for_borrow( diff --git a/src/librustc_mir/build/expr/as_place.rs b/src/librustc_mir/build/expr/as_place.rs index 8d2bef39bed42..d3e013acc9e3a 100644 --- a/src/librustc_mir/build/expr/as_place.rs +++ b/src/librustc_mir/build/expr/as_place.rs @@ -4,9 +4,11 @@ use crate::build::expr::category::Category; use crate::build::ForGuard::{OutsideGuard, RefWithinGuard}; use crate::build::{BlockAnd, BlockAndExtension, Builder}; use crate::hair::*; +use rustc::middle::region; use rustc::mir::interpret::{PanicInfo::BoundsCheck}; use rustc::mir::*; -use rustc::ty::{CanonicalUserTypeAnnotation, Ty, TyCtxt, Variance}; +use rustc::ty::{self, CanonicalUserTypeAnnotation, Ty, TyCtxt, Variance}; +use syntax_pos::Span; use rustc_index::vec::Idx; @@ -68,6 +70,17 @@ impl From> for PlaceBuilder<'tcx> { impl<'a, 'tcx> Builder<'a, 'tcx> { /// Compile `expr`, yielding a place that we can move from etc. + /// + /// WARNING: Any user code might: + /// * Invalidate any slice bounds checks performed. + /// * Change the address that this `Place` refers to. + /// * Modify the memory that this place refers to. + /// * Invalidate the memory that this place refers to, this will be caught + /// by borrow checking. + /// + /// Extra care is needed if any user code is allowed to run between calling + /// this method and using it, as is the case for `match` and index + /// expressions. pub fn as_place(&mut self, mut block: BasicBlock, expr: M) -> BlockAnd> where M: Mirror<'tcx, Output = Expr<'tcx>>, @@ -83,7 +96,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { M: Mirror<'tcx, Output = Expr<'tcx>>, { let expr = self.hir.mirror(expr); - self.expr_as_place(block, expr, Mutability::Mut) + self.expr_as_place(block, expr, Mutability::Mut, None) } /// Compile `expr`, yielding a place that we can move from etc. @@ -114,7 +127,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { M: Mirror<'tcx, Output = Expr<'tcx>>, { let expr = self.hir.mirror(expr); - self.expr_as_place(block, expr, Mutability::Not) + self.expr_as_place(block, expr, Mutability::Not, None) } fn expr_as_place( @@ -122,6 +135,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { mut block: BasicBlock, expr: Expr<'tcx>, mutability: Mutability, + fake_borrow_temps: Option<&mut Vec>, ) -> BlockAnd> { debug!( "expr_as_place(block={:?}, expr={:?}, mutability={:?})", @@ -137,63 +151,40 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { lint_level, value, } => this.in_scope((region_scope, source_info), lint_level, |this| { - if mutability == Mutability::Not { - this.as_read_only_place_builder(block, value) - } else { - this.as_place_builder(block, value) - } + let value = this.hir.mirror(value); + this.expr_as_place(block, value, mutability, fake_borrow_temps) }), ExprKind::Field { lhs, name } => { - let place_builder = unpack!(block = this.as_place_builder(block, lhs)); + let lhs = this.hir.mirror(lhs); + let place_builder = unpack!(block = this.expr_as_place( + block, + lhs, + mutability, + fake_borrow_temps, + )); block.and(place_builder.field(name, expr.ty)) } ExprKind::Deref { arg } => { - let place_builder = unpack!(block = this.as_place_builder(block, arg)); + let arg = this.hir.mirror(arg); + let place_builder = unpack!(block = this.expr_as_place( + block, + arg, + mutability, + fake_borrow_temps, + )); block.and(place_builder.deref()) } ExprKind::Index { lhs, index } => { - let (usize_ty, bool_ty) = (this.hir.usize_ty(), this.hir.bool_ty()); - - let place_builder = unpack!(block = this.as_place_builder(block, lhs)); - // Making this a *fresh* temporary also means we do not have to worry about - // the index changing later: Nothing will ever change this temporary. - // The "retagging" transformation (for Stacked Borrows) relies on this. - let idx = unpack!(block = this.as_temp( + this.lower_index_expression( block, - expr.temp_lifetime, + lhs, index, - Mutability::Not, - )); - - let slice = place_builder.clone().into_place(this.hir.tcx()); - // bounds check: - let (len, lt) = ( - this.temp(usize_ty.clone(), expr_span), - this.temp(bool_ty, expr_span), - ); - this.cfg.push_assign( - block, - source_info, // len = len(slice) - &len, - Rvalue::Len(slice), - ); - this.cfg.push_assign( - block, - source_info, // lt = idx < len - <, - Rvalue::BinaryOp( - BinOp::Lt, - Operand::Copy(Place::from(idx)), - Operand::Copy(len.clone()), - ), - ); - - let msg = BoundsCheck { - len: Operand::Move(len), - index: Operand::Copy(Place::from(idx)), - }; - let success = this.assert(block, Operand::Move(lt), true, msg, expr_span); - success.and(place_builder.index(idx)) + mutability, + fake_borrow_temps, + expr.temp_lifetime, + expr_span, + source_info, + ) } ExprKind::SelfRef => block.and(PlaceBuilder::from(Local::new(1))), ExprKind::VarRef { id } => { @@ -215,7 +206,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { )), ExprKind::PlaceTypeAscription { source, user_ty } => { - let place_builder = unpack!(block = this.as_place_builder(block, source)); + let source = this.hir.mirror(source); + let place_builder = unpack!(block = this.expr_as_place( + block, + source, + mutability, + fake_borrow_temps, + )); if let Some(user_ty) = user_ty { let annotation_index = this.canonical_user_type_annotations.push( CanonicalUserTypeAnnotation { @@ -309,4 +306,208 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } } + + /// Lower an index expression + /// + /// This has two complications; + /// + /// * We need to do a bounds check. + /// * We need to ensure that the bounds check can't be invalidated using an + /// expression like `x[1][{x = y; 2}]`. We use fake borrows here to ensure + /// that this is the case. + fn lower_index_expression( + &mut self, + mut block: BasicBlock, + base: ExprRef<'tcx>, + index: ExprRef<'tcx>, + mutability: Mutability, + fake_borrow_temps: Option<&mut Vec>, + temp_lifetime: Option, + expr_span: Span, + source_info: SourceInfo + ) -> BlockAnd> { + let lhs = self.hir.mirror(base); + + let base_fake_borrow_temps = &mut Vec::new(); + let is_outermost_index = fake_borrow_temps.is_none(); + let fake_borrow_temps = fake_borrow_temps.unwrap_or(base_fake_borrow_temps); + + let base_place = unpack!(block = self.expr_as_place( + block, + lhs, + mutability, + Some(fake_borrow_temps), + )); + + // Making this a *fresh* temporary means we do not have to worry about + // the index changing later: Nothing will ever change this temporary. + // The "retagging" transformation (for Stacked Borrows) relies on this. + let idx = unpack!(block = self.as_temp( + block, + temp_lifetime, + index, + Mutability::Not, + )); + + block = self.bounds_check( + block, + base_place.clone().into_place(self.hir.tcx()), + idx, + expr_span, + source_info, + ); + + if is_outermost_index { + self.read_fake_borrows(block, fake_borrow_temps, source_info) + } else { + self.add_fake_borrows_of_base( + &base_place, + block, + fake_borrow_temps, + expr_span, + source_info, + ); + } + + block.and(base_place.index(idx)) + } + + fn bounds_check( + &mut self, + block: BasicBlock, + slice: Place<'tcx>, + index: Local, + expr_span: Span, + source_info: SourceInfo, + ) -> BasicBlock { + let usize_ty = self.hir.usize_ty(); + let bool_ty = self.hir.bool_ty(); + // bounds check: + let len = self.temp(usize_ty, expr_span); + let lt = self.temp(bool_ty, expr_span); + + // len = len(slice) + self.cfg.push_assign( + block, + source_info, + &len, + Rvalue::Len(slice), + ); + // lt = idx < len + self.cfg.push_assign( + block, + source_info, + <, + Rvalue::BinaryOp( + BinOp::Lt, + Operand::Copy(Place::from(index)), + Operand::Copy(len.clone()), + ), + ); + let msg = BoundsCheck { + len: Operand::Move(len), + index: Operand::Copy(Place::from(index)), + }; + // assert!(lt, "...") + self.assert(block, Operand::Move(lt), true, msg, expr_span) + } + + fn add_fake_borrows_of_base( + &mut self, + base_place: &PlaceBuilder<'tcx>, + block: BasicBlock, + fake_borrow_temps: &mut Vec, + expr_span: Span, + source_info: SourceInfo, + ) { + let tcx = self.hir.tcx(); + let place_ty = Place::ty_from( + &base_place.base, + &base_place.projection, + &self.local_decls, + tcx, + ); + if let ty::Slice(_) = place_ty.ty.kind { + // We need to create fake borrows to ensure that the bounds + // check that we just did stays valid. Since we can't assign to + // unsized values, we only need to ensure that none of the + // pointers in the base place are modified. + for (idx, elem) in base_place.projection.iter().enumerate().rev() { + match elem { + ProjectionElem::Deref => { + let fake_borrow_deref_ty = Place::ty_from( + &base_place.base, + &base_place.projection[..idx], + &self.local_decls, + tcx, + ).ty; + let fake_borrow_ty = tcx.mk_imm_ref( + tcx.lifetimes.re_erased, + fake_borrow_deref_ty, + ); + let fake_borrow_temp = self.local_decls.push( + LocalDecl::new_temp(fake_borrow_ty, expr_span) + ); + let projection = tcx.intern_place_elems(&base_place.projection[..idx]); + self.cfg.push_assign( + block, + source_info, + &fake_borrow_temp.into(), + Rvalue::Ref( + tcx.lifetimes.re_erased, + BorrowKind::Shallow, + Place { + base: base_place.base.clone(), + projection, + } + ), + ); + fake_borrow_temps.push(fake_borrow_temp); + } + ProjectionElem::Index(_) => { + let index_ty = Place::ty_from( + &base_place.base, + &base_place.projection[..idx], + &self.local_decls, + tcx, + ); + match index_ty.ty.kind { + // The previous index expression has already + // done any index expressions needed here. + ty::Slice(_) => break, + ty::Array(..) => (), + _ => bug!("unexpected index base"), + } + } + ProjectionElem::Field(..) + | ProjectionElem::Downcast(..) + | ProjectionElem::ConstantIndex { .. } + | ProjectionElem::Subslice { .. } => (), + } + } + } + } + + fn read_fake_borrows( + &mut self, + block: BasicBlock, + fake_borrow_temps: &mut Vec, + source_info: SourceInfo, + ) { + // All indexes have been evaluated now, read all of the + // fake borrows so that they are live across those index + // expressions. + for temp in fake_borrow_temps { + self.cfg.push( + block, + Statement { + source_info, + kind: StatementKind::FakeRead( + FakeReadCause::ForIndex, + Box::new(Place::from(*temp)), + ) + } + ); + } + } } diff --git a/src/librustc_mir/util/borrowck_errors.rs b/src/librustc_mir/util/borrowck_errors.rs index 96ba829358280..bf01ad1a0236f 100644 --- a/src/librustc_mir/util/borrowck_errors.rs +++ b/src/librustc_mir/util/borrowck_errors.rs @@ -395,23 +395,25 @@ impl<'cx, 'tcx> crate::borrow_check::MirBorrowckCtxt<'cx, 'tcx> { ) } - crate fn cannot_mutate_in_match_guard( + crate fn cannot_mutate_in_immutable_section( &self, mutate_span: Span, - match_span: Span, - match_place: &str, + immutable_span: Span, + immutable_place: &str, + immutable_section: &str, action: &str, ) -> DiagnosticBuilder<'cx> { let mut err = struct_span_err!( self, mutate_span, E0510, - "cannot {} `{}` in match guard", + "cannot {} `{}` in {}", action, - match_place, + immutable_place, + immutable_section, ); err.span_label(mutate_span, format!("cannot {}", action)); - err.span_label(match_span, String::from("value is immutable in match guard")); + err.span_label(immutable_span, format!("value is immutable in {}", immutable_section)); err } diff --git a/src/test/mir-opt/storage_live_dead_in_statics.rs b/src/test/mir-opt/storage_live_dead_in_statics.rs index 2ed34ecfad2c6..5dc15286bab50 100644 --- a/src/test/mir-opt/storage_live_dead_in_statics.rs +++ b/src/test/mir-opt/storage_live_dead_in_statics.rs @@ -36,11 +36,11 @@ fn main() { // END RUST SOURCE // START rustc.XXX.mir_map.0.mir // let mut _0: &'static Foo; -// let mut _1: &'static Foo; +// let _1: &'static Foo; // let _2: Foo; // let mut _3: &'static [(u32, u32)]; // let mut _4: &'static [(u32, u32); 42]; -// let mut _5: &'static [(u32, u32); 42]; +// let _5: &'static [(u32, u32); 42]; // let _6: [(u32, u32); 42]; // let mut _7: (u32, u32); // let mut _8: (u32, u32); diff --git a/src/test/mir-opt/uninhabited_enum_branching.rs b/src/test/mir-opt/uninhabited_enum_branching.rs index 1f37ff1498d9b..aa56918a9b8c1 100644 --- a/src/test/mir-opt/uninhabited_enum_branching.rs +++ b/src/test/mir-opt/uninhabited_enum_branching.rs @@ -34,12 +34,12 @@ fn main() { // let _1: &str; // let mut _2: Test1; // let mut _3: isize; -// let mut _4: &str; -// let mut _5: &str; +// let _4: &str; +// let _5: &str; // let _6: &str; // let mut _7: Test2; // let mut _8: isize; -// let mut _9: &str; +// let _9: &str; // bb0: { // StorageLive(_1); // StorageLive(_2); @@ -103,12 +103,12 @@ fn main() { // let _1: &str; // let mut _2: Test1; // let mut _3: isize; -// let mut _4: &str; -// let mut _5: &str; +// let _4: &str; +// let _5: &str; // let _6: &str; // let mut _7: Test2; // let mut _8: isize; -// let mut _9: &str; +// let _9: &str; // bb0: { // StorageLive(_1); // StorageLive(_2); @@ -172,12 +172,12 @@ fn main() { // let _1: &str; // let mut _2: Test1; // let mut _3: isize; -// let mut _4: &str; -// let mut _5: &str; +// let _4: &str; +// let _5: &str; // let _6: &str; // let mut _7: Test2; // let mut _8: isize; -// let mut _9: &str; +// let _9: &str; // bb0: { // StorageLive(_1); // StorageLive(_2); diff --git a/src/test/ui/borrowck/slice-index-bounds-check-invalidation.rs b/src/test/ui/borrowck/slice-index-bounds-check-invalidation.rs new file mode 100644 index 0000000000000..0e0e3cda6e2ed --- /dev/null +++ b/src/test/ui/borrowck/slice-index-bounds-check-invalidation.rs @@ -0,0 +1,82 @@ +// Test that we error if a slice is modified after it has been bounds checked +// and before we actually index it. + +fn modify_before_assert_slice_slice(x: &[&[i32]]) -> i32 { + let mut x = x; + let z: &[i32] = &[1, 2, 3]; + let y: &[&[i32]] = &[z]; + x[{ x = y; 0 }][2] // OK we haven't checked any bounds before we index `x`. +} + +fn modify_before_assert_array_slice(x: &[&[i32]; 3]) -> i32 { + let mut x = x; + let z: &[i32] = &[1, 2, 3]; + let y: &[&[i32]; 3] = &[z, z, z]; + x[{ x = y; 0 }][2] // OK we haven't checked any bounds before we index `x`. +} + +fn modify_before_assert_slice_array(x: &[&[i32; 3]]) -> i32 { + let mut x = x; + let z: &[i32; 3] = &[1, 2, 3]; + let y: &[&[i32; 3]] = &[z]; + x[{ x = y; 0 }][2] // OK we haven't checked any bounds before we index `x`. +} + +fn modify_before_assert_array_array(x: &[&[i32; 3]; 3]) -> i32 { + let mut x = x; + let z: &[i32; 3] = &[1, 2, 3]; + let y: &[&[i32; 3]; 3] = &[z, z, z]; + x[{ x = y; 0 }][2] // OK we haven't checked any bounds before we index `x`. +} + +fn modify_after_assert_slice_slice(x: &[&[i32]]) -> i32 { + let mut x = x; + let z: &[i32] = &[1, 2, 3]; + let y: &[&[i32]] = &[&z]; + x[1][{ x = y; 2}] //~ ERROR cannot assign `x` in indexing expression +} + +fn modify_after_assert_array_slice(x: &[&[i32]; 1]) -> i32 { + let mut x = x; + let z: &[i32] = &[1, 2, 3]; + let y: &[&[i32]; 1] = &[&z]; + x[0][{ x = y; 2}] // OK cannot invalidate a fixed-size array bounds check +} + +fn modify_after_assert_slice_array(x: &[&[i32; 3]]) -> i32 { + let mut x = x; + let z: &[i32; 3] = &[1, 2, 3]; + let y: &[&[i32; 3]] = &[&z]; + x[1][{ x = y; 2}] //~ ERROR cannot assign `x` in indexing expression +} + +fn modify_after_assert_array_array(x: &[&[i32; 3]; 1]) -> i32 { + let mut x = x; + let z: &[i32; 3] = &[1, 2, 3]; + let y: &[&[i32; 3]; 1] = &[&z]; + x[0][{ x = y; 2}] // OK cannot invalidate a fixed-size array bounds check +} + +fn modify_after_assert_slice_slice_array(x: &[&[[i32; 1]]]) -> i32 { + let mut x = x; + let z: &[[i32; 1]] = &[[1], [2], [3]]; + let y: &[&[[i32; 1]]] = &[&z]; + x[1][{ x = y; 2}][0] //~ ERROR cannot assign `x` in indexing expression +} + +fn modify_after_assert_slice_slice_slice(x: &[&[&[i32]]]) -> i32 { + let mut x = x; + let z: &[&[i32]] = &[&[1], &[2], &[3]]; + let y: &[&[&[i32]]] = &[z]; + x[1][{ x = y; 2}][0] //~ ERROR cannot assign `x` in indexing expression +} + + +fn main() { + println!("{}", modify_after_assert_slice_array(&[&[4, 5, 6], &[9, 10, 11]])); + println!("{}", modify_after_assert_slice_slice(&[&[4, 5, 6], &[9, 10, 11]])); + println!("{}", modify_after_assert_slice_slice_array(&[&[[4], [5], [6]], &[[9], [10], [11]]])); + println!("{}", modify_after_assert_slice_slice_slice( + &[&[&[4], &[5], &[6]], &[&[9], &[10], &[11]]]), + ); +} diff --git a/src/test/ui/borrowck/slice-index-bounds-check-invalidation.stderr b/src/test/ui/borrowck/slice-index-bounds-check-invalidation.stderr new file mode 100644 index 0000000000000..f9ed16f19cd69 --- /dev/null +++ b/src/test/ui/borrowck/slice-index-bounds-check-invalidation.stderr @@ -0,0 +1,35 @@ +error[E0510]: cannot assign `x` in indexing expression + --> $DIR/slice-index-bounds-check-invalidation.rs:36:12 + | +LL | x[1][{ x = y; 2}] + | ---- ^^^^^ cannot assign + | | + | value is immutable in indexing expression + +error[E0510]: cannot assign `x` in indexing expression + --> $DIR/slice-index-bounds-check-invalidation.rs:50:12 + | +LL | x[1][{ x = y; 2}] + | ---- ^^^^^ cannot assign + | | + | value is immutable in indexing expression + +error[E0510]: cannot assign `x` in indexing expression + --> $DIR/slice-index-bounds-check-invalidation.rs:64:12 + | +LL | x[1][{ x = y; 2}][0] + | ---- ^^^^^ cannot assign + | | + | value is immutable in indexing expression + +error[E0510]: cannot assign `x` in indexing expression + --> $DIR/slice-index-bounds-check-invalidation.rs:71:12 + | +LL | x[1][{ x = y; 2}][0] + | ---- ^^^^^ cannot assign + | | + | value is immutable in indexing expression + +error: aborting due to 4 previous errors + +For more information about this error, try `rustc --explain E0510`. From 4bf0685cca167e684340152809be20a16ad65a76 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sat, 19 Oct 2019 21:01:36 +0100 Subject: [PATCH 3/3] Evaluate borrow and struct expressions in `into` This fixes some ordering problems around assignment expressions. --- src/librustc_mir/build/expr/as_rvalue.rs | 95 ++-------------- src/librustc_mir/build/expr/category.rs | 4 +- src/librustc_mir/build/expr/into.rs | 102 +++++++++++++++++- .../ui/borrowck/borrowck-init-in-fru.stderr | 4 +- src/test/ui/mir/mir_assign_eval_order.rs | 67 ++++++++++++ src/test/ui/nll/issue-52534-2.stderr | 4 +- src/test/ui/span/issue-36537.stderr | 4 +- 7 files changed, 184 insertions(+), 96 deletions(-) create mode 100644 src/test/ui/mir/mir_assign_eval_order.rs diff --git a/src/librustc_mir/build/expr/as_rvalue.rs b/src/librustc_mir/build/expr/as_rvalue.rs index 4f1ac8e51dc20..f9b77a4b5dd2a 100644 --- a/src/librustc_mir/build/expr/as_rvalue.rs +++ b/src/librustc_mir/build/expr/as_rvalue.rs @@ -1,6 +1,5 @@ //! See docs in `build/expr/mod.rs`. -use rustc_data_structures::fx::FxHashMap; use rustc_index::vec::Idx; use crate::build::expr::category::{Category, RvalueFunc}; @@ -9,11 +8,16 @@ use crate::hair::*; use rustc::middle::region; use rustc::mir::interpret::PanicInfo; use rustc::mir::*; -use rustc::ty::{self, CanonicalUserTypeAnnotation, Ty, UpvarSubsts}; +use rustc::ty::{self, Ty, UpvarSubsts}; use syntax_pos::Span; impl<'a, 'tcx> Builder<'a, 'tcx> { - /// See comment on `as_local_operand` + /// Returns an rvalue suitable for use until the end of the current + /// scope expression. + /// + /// The operand returned from this function will *not be valid* after + /// an ExprKind::Scope is passed, so please do *not* return it from + /// functions to avoid bad miscompiles. pub fn as_local_rvalue(&mut self, block: BasicBlock, expr: M) -> BlockAnd> where M: Mirror<'tcx, Output = Expr<'tcx>>, @@ -23,7 +27,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } /// Compile `expr`, yielding an rvalue. - pub fn as_rvalue( + fn as_rvalue( &mut self, block: BasicBlock, scope: Option, @@ -66,16 +70,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let value_operand = unpack!(block = this.as_operand(block, scope, value)); block.and(Rvalue::Repeat(value_operand, count)) } - ExprKind::Borrow { - borrow_kind, - arg, - } => { - let arg_place = match borrow_kind { - BorrowKind::Shared => unpack!(block = this.as_read_only_place(block, arg)), - _ => unpack!(block = this.as_place(block, arg)), - }; - block.and(Rvalue::Ref(this.hir.tcx().lifetimes.re_erased, borrow_kind, arg_place)) - } ExprKind::Binary { op, lhs, rhs } => { let lhs = unpack!(block = this.as_operand(block, scope, lhs)); let rhs = unpack!(block = this.as_operand(block, scope, rhs)); @@ -256,77 +250,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { }; block.and(Rvalue::Aggregate(result, operands)) } - ExprKind::Adt { - adt_def, - variant_index, - substs, - user_ty, - fields, - base, - } => { - // see (*) above - let is_union = adt_def.is_union(); - let active_field_index = if is_union { - Some(fields[0].name.index()) - } else { - None - }; - - // first process the set of fields that were provided - // (evaluating them in order given by user) - let fields_map: FxHashMap<_, _> = fields - .into_iter() - .map(|f| { - ( - f.name, - unpack!(block = this.as_operand(block, scope, f.expr)), - ) - }).collect(); - - let field_names = this.hir.all_fields(adt_def, variant_index); - - let fields = if let Some(FruInfo { base, field_types }) = base { - let base = unpack!(block = this.as_place(block, base)); - - // MIR does not natively support FRU, so for each - // base-supplied field, generate an operand that - // reads it from the base. - field_names - .into_iter() - .zip(field_types.into_iter()) - .map(|(n, ty)| match fields_map.get(&n) { - Some(v) => v.clone(), - None => this.consume_by_copy_or_move(this.hir.tcx().mk_place_field( - base.clone(), - n, - ty, - )), - }) - .collect() - } else { - field_names - .iter() - .filter_map(|n| fields_map.get(n).cloned()) - .collect() - }; - - let inferred_ty = expr.ty; - let user_ty = user_ty.map(|ty| { - this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation { - span: source_info.span, - user_ty: ty, - inferred_ty, - }) - }); - let adt = box AggregateKind::Adt( - adt_def, - variant_index, - substs, - user_ty, - active_field_index, - ); - block.and(Rvalue::Aggregate(adt, fields)) - } ExprKind::Assign { .. } | ExprKind::AssignOp { .. } => { block = unpack!(this.stmt_expr(block, expr, None)); block.and(this.unit_rvalue()) @@ -351,6 +274,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { | ExprKind::Match { .. } | ExprKind::NeverToAny { .. } | ExprKind::Use { .. } + | ExprKind::Borrow { .. } + | ExprKind::Adt { .. } | ExprKind::Loop { .. } | ExprKind::LogicalOp { .. } | ExprKind::Call { .. } diff --git a/src/librustc_mir/build/expr/category.rs b/src/librustc_mir/build/expr/category.rs index f679a00035d76..ae5289986e77c 100644 --- a/src/librustc_mir/build/expr/category.rs +++ b/src/librustc_mir/build/expr/category.rs @@ -48,11 +48,12 @@ impl Category { | ExprKind::Match { .. } | ExprKind::NeverToAny { .. } | ExprKind::Use { .. } + | ExprKind::Adt { .. } + | ExprKind::Borrow { .. } | ExprKind::Call { .. } => Some(Category::Rvalue(RvalueFunc::Into)), ExprKind::Array { .. } | ExprKind::Tuple { .. } - | ExprKind::Adt { .. } | ExprKind::Closure { .. } | ExprKind::Unary { .. } | ExprKind::Binary { .. } @@ -60,7 +61,6 @@ impl Category { | ExprKind::Cast { .. } | ExprKind::Pointer { .. } | ExprKind::Repeat { .. } - | ExprKind::Borrow { .. } | ExprKind::Assign { .. } | ExprKind::AssignOp { .. } | ExprKind::Yield { .. } diff --git a/src/librustc_mir/build/expr/into.rs b/src/librustc_mir/build/expr/into.rs index c9e5fca4d75e7..404ca3204e6c0 100644 --- a/src/librustc_mir/build/expr/into.rs +++ b/src/librustc_mir/build/expr/into.rs @@ -4,7 +4,9 @@ use crate::build::expr::category::{Category, RvalueFunc}; use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder}; use crate::hair::*; use rustc::mir::*; -use rustc::ty; +use rustc::ty::{self, CanonicalUserTypeAnnotation}; +use rustc_data_structures::fx::FxHashMap; +use syntax_pos::symbol::sym; use rustc_target::spec::abi::Abi; @@ -270,6 +272,102 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ExprKind::Use { source } => { this.into(destination, block, source) } + ExprKind::Borrow { arg, borrow_kind } => { + // We don't do this in `as_rvalue` because we use `as_place` + // for borrow expressions, so we cannot create an `RValue` that + // remains valid across user code. `as_rvalue` is usually called + // by this method anyway, so this shouldn't cause too many + // unnecessary temporaries. + let arg_place = match borrow_kind { + BorrowKind::Shared => unpack!(block = this.as_read_only_place(block, arg)), + _ => unpack!(block = this.as_place(block, arg)), + }; + let borrow = Rvalue::Ref( + this.hir.tcx().lifetimes.re_erased, + borrow_kind, + arg_place, + ); + this.cfg.push_assign(block, source_info, destination, borrow); + block.unit() + } + ExprKind::Adt { + adt_def, + variant_index, + substs, + user_ty, + fields, + base, + } => { + // See the notes for `ExprKind::Array` in `as_rvalue` and for + // `ExprKind::Borrow` above. + let is_union = adt_def.is_union(); + let active_field_index = if is_union { + Some(fields[0].name.index()) + } else { + None + }; + + let scope = this.local_scope(); + + // first process the set of fields that were provided + // (evaluating them in order given by user) + let fields_map: FxHashMap<_, _> = fields + .into_iter() + .map(|f| { + ( + f.name, + unpack!(block = this.as_operand(block, scope, f.expr)), + ) + }).collect(); + + let field_names = this.hir.all_fields(adt_def, variant_index); + + let fields = if let Some(FruInfo { base, field_types }) = base { + let base = unpack!(block = this.as_place(block, base)); + + // MIR does not natively support FRU, so for each + // base-supplied field, generate an operand that + // reads it from the base. + field_names + .into_iter() + .zip(field_types.into_iter()) + .map(|(n, ty)| match fields_map.get(&n) { + Some(v) => v.clone(), + None => this.consume_by_copy_or_move( + this.hir.tcx().mk_place_field(base.clone(), n, ty), + ), + }).collect() + } else { + field_names + .iter() + .filter_map(|n| fields_map.get(n).cloned()) + .collect() + }; + + let inferred_ty = expr.ty; + let user_ty = user_ty.map(|ty| { + this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation { + span: source_info.span, + user_ty: ty, + inferred_ty, + }) + }); + let adt = box AggregateKind::Adt( + adt_def, + variant_index, + substs, + user_ty, + active_field_index, + ); + this.cfg.push_assign( + block, + source_info, + destination, + Rvalue::Aggregate(adt, fields) + ); + block.unit() + } + // These cases don't actually need a destination ExprKind::Assign { .. } @@ -324,10 +422,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { | ExprKind::Cast { .. } | ExprKind::Pointer { .. } | ExprKind::Repeat { .. } - | ExprKind::Borrow { .. } | ExprKind::Array { .. } | ExprKind::Tuple { .. } - | ExprKind::Adt { .. } | ExprKind::Closure { .. } | ExprKind::Literal { .. } | ExprKind::Yield { .. } => { diff --git a/src/test/ui/borrowck/borrowck-init-in-fru.stderr b/src/test/ui/borrowck/borrowck-init-in-fru.stderr index a4c042d1c125f..f01afe1466aef 100644 --- a/src/test/ui/borrowck/borrowck-init-in-fru.stderr +++ b/src/test/ui/borrowck/borrowck-init-in-fru.stderr @@ -1,8 +1,8 @@ error[E0381]: use of possibly-uninitialized variable: `origin` - --> $DIR/borrowck-init-in-fru.rs:9:5 + --> $DIR/borrowck-init-in-fru.rs:9:14 | LL | origin = Point { x: 10, ..origin }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of possibly-uninitialized `origin.y` + | ^^^^^^^^^^^^^^^^^^^^^^^^^ use of possibly-uninitialized `origin.y` error: aborting due to previous error diff --git a/src/test/ui/mir/mir_assign_eval_order.rs b/src/test/ui/mir/mir_assign_eval_order.rs new file mode 100644 index 0000000000000..1594421b0b13b --- /dev/null +++ b/src/test/ui/mir/mir_assign_eval_order.rs @@ -0,0 +1,67 @@ +// Test evaluation order of assignment expressions is right to left. + +// run-pass + +// We would previously not finish evaluating borrow and FRU expressions before +// starting on the LHS + +struct S(i32); + +fn evaluate_reborrow_before_assign() { + let mut x = &1; + let y = &mut &2; + let z = &3; + // There's an implicit reborrow of `x` on the right-hand side of the + // assignement. Note that writing an explicit reborrow would not show this + // bug, as now there would be two reborrows on the right-hand side and at + // least one of them would happen before the left-hand side is evaluated. + *{ x = z; &mut *y } = x; + assert_eq!(*x, 3); + assert_eq!(**y, 1); // y should be assigned the original value of `x`. +} + +fn evaluate_mut_reborrow_before_assign() { + let mut x = &mut 1; + let y = &mut &mut 2; + let z = &mut 3; + *{ x = z; &mut *y } = x; + assert_eq!(*x, 3); + assert_eq!(**y, 1); // y should be assigned the original value of `x`. +} + +// We should evaluate `x[2]` and borrow the value out *before* evaluating the +// LHS and changing its value. +fn evaluate_ref_to_temp_before_assign_slice() { + let mut x = &[S(0), S(1), S(2)][..]; + let y = &mut &S(7); + *{ x = &[S(3), S(4), S(5)]; &mut *y } = &x[2]; + assert_eq!(2, y.0); + assert_eq!(5, x[2].0); +} + +// We should evaluate `x[2]` and copy the value out *before* evaluating the LHS +// and changing its value. +fn evaluate_fru_to_temp_before_assign_slice() { + let mut x = &[S(0), S(1), S(2)][..]; + let y = &mut S(7); + *{ x = &[S(3), S(4), S(5)]; &mut *y } = S { ..x[2] }; + assert_eq!(2, y.0); + assert_eq!(5, x[2].0); +} + +// We should evaluate `*x` and copy the value out *before* evaluating the LHS +// and dropping `x`. +fn evaluate_fru_to_temp_before_assign_box() { + let x = Box::new(S(0)); + let y = &mut S(1); + *{ drop(x); &mut *y } = S { ..*x }; + assert_eq!(0, y.0); +} + +fn main() { + evaluate_reborrow_before_assign(); + evaluate_mut_reborrow_before_assign(); + evaluate_ref_to_temp_before_assign_slice(); + evaluate_fru_to_temp_before_assign_slice(); + evaluate_fru_to_temp_before_assign_box(); +} diff --git a/src/test/ui/nll/issue-52534-2.stderr b/src/test/ui/nll/issue-52534-2.stderr index dd8a87f7e29aa..cef4aba024037 100644 --- a/src/test/ui/nll/issue-52534-2.stderr +++ b/src/test/ui/nll/issue-52534-2.stderr @@ -1,8 +1,8 @@ error[E0597]: `x` does not live long enough - --> $DIR/issue-52534-2.rs:6:9 + --> $DIR/issue-52534-2.rs:6:13 | LL | y = &x - | ^^^^^^ borrowed value does not live long enough + | ^^ borrowed value does not live long enough LL | LL | } | - `x` dropped here while still borrowed diff --git a/src/test/ui/span/issue-36537.stderr b/src/test/ui/span/issue-36537.stderr index edb804e850e2c..0939584380af9 100644 --- a/src/test/ui/span/issue-36537.stderr +++ b/src/test/ui/span/issue-36537.stderr @@ -1,8 +1,8 @@ error[E0597]: `a` does not live long enough - --> $DIR/issue-36537.rs:5:9 + --> $DIR/issue-36537.rs:5:13 | LL | p = &a; - | ^^^^^^ borrowed value does not live long enough + | ^^ borrowed value does not live long enough ... LL | } | - `a` dropped here while still borrowed