Skip to content

Commit

Permalink
Fix soundness issue with index bounds checks
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@matthewjasper
matthewjasper committed Nov 11, 2019
1 parent 2ff89c5 commit 7320818
Show file tree
Hide file tree
Showing 8 changed files with 500 additions and 124 deletions.
14 changes: 11 additions & 3 deletions src/librustc/mir/mod.rs
View file
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
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

0 comments on commit 7320818

Please sign in to comment.