Skip to content

Fix MIR lowering evaluation order and soundness bug #65608

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 12, 2019
Merged

Fix MIR lowering evaluation order and soundness bug #65608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ff89c5
Avoid a string comparison in MIR construction
matthewjasper May 26, 2019
7320818
Fix soundness issue with index bounds checks
matthewjasper Oct 19, 2019
4bf0685
Evaluate borrow and struct expressions in `into`
matthewjasper Oct 19, 2019
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
14 changes: 11 additions & 3 deletions src/librustc/mir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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<V, T> {
Deref,
Field(Field, T),
Expand Down
154 changes: 101 additions & 53 deletions src/librustc_mir/borrow_check/conflict_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -469,42 +485,42 @@ 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,
second_borrow_desc,
)
}

(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,
second_borrow_desc,
)
}

(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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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<FakeReadCause>,
}
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(
Expand Down
Loading