Skip to content

Commit

Permalink
librustc: Require lifetimes within the types of values with destructors
Browse files Browse the repository at this point in the history
to have strictly greater lifetime than the values themselves.

Additionally, remove `#[unsafe_destructor]` in favor of these new
restrictions.

This broke a fair amount of code that had to do with `RefCell`s. We will
probably need to do some work to improve the resulting ergonomics. Most
code that broke looked like:

    match foo.bar.borrow().baz { ... }
    use_foo(&mut foo);

Change this code to:

    {
        let bar = foo.bar.borrow();
        match bar.baz { ... }
    }
    use_foo(&mut foo);

This fixes an important memory safety hole relating to destructors,
represented by issue #8861.

[breaking-change]
  • Loading branch information
pcwalton committed Sep 16, 2014
1 parent 828e075 commit 1b9a16c
Show file tree
Hide file tree
Showing 42 changed files with 1,160 additions and 645 deletions.
36 changes: 19 additions & 17 deletions src/libcollections/treemap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1084,27 +1084,29 @@ impl<T: Ord> Set<T> for TreeSet<T> {
}

fn is_subset(&self, other: &TreeSet<T>) -> bool {
let mut x = self.iter();
let mut y = other.iter();
let mut a = x.next();
let mut b = y.next();
while a.is_some() {
if b.is_none() {
return false;
}
{
let mut x = self.iter();
let mut y = other.iter();
let mut a = x.next();
let mut b = y.next();
while a.is_some() {
if b.is_none() {
return false;
}

let a1 = a.unwrap();
let b1 = b.unwrap();
let a1 = a.unwrap();
let b1 = b.unwrap();

match b1.cmp(a1) {
Less => (),
Greater => return false,
Equal => a = x.next(),
}
match b1.cmp(a1) {
Less => (),
Greater => return false,
Equal => a = x.next(),
}

b = y.next();
b = y.next();
}
true
}
true
}
}

Expand Down
7 changes: 5 additions & 2 deletions src/libcore/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,11 @@ impl<T> RefCell<T> {

#[unstable = "waiting for `Clone` to become stable"]
impl<T: Clone> Clone for RefCell<T> {
fn clone(&self) -> RefCell<T> {
RefCell::new(self.borrow().clone())
fn clone<'a>(&'a self) -> RefCell<T> {
{
let borrowed: Ref<'a,T> = self.borrow();
RefCell::new(borrowed.clone())
}
}
}

Expand Down
26 changes: 16 additions & 10 deletions src/libcore/finally.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,22 @@ impl<T> Finally<T> for fn() -> T {
* })
* ```
*/
pub fn try_finally<T,U,R>(mutate: &mut T,
drop: U,
try_fn: |&mut T, U| -> R,
finally_fn: |&mut T|)
-> R {
let f = Finallyalizer {
mutate: mutate,
dtor: finally_fn,
};
try_fn(&mut *f.mutate, drop)
pub fn try_finally<'a,
T,
U,
R>(
mutate: &'a mut T,
drop: U,
try_fn: |&mut T, U| -> R,
finally_fn: |&mut T|:'a)
-> R {
{
let f: Finallyalizer<'a,T> = Finallyalizer {
mutate: mutate,
dtor: finally_fn,
};
try_fn(&mut *f.mutate, drop)
}
}

struct Finallyalizer<'a,A:'a> {
Expand Down
9 changes: 6 additions & 3 deletions src/libgreen/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,13 @@ impl BasicLoop {

fn remote_work(&mut self) {
let messages = unsafe {
mem::replace(&mut *self.messages.lock(), Vec::new())
let lock = &mut *self.messages.lock();
mem::replace(lock, Vec::new())
};
for message in messages.move_iter() {
self.message(message);
{
for message in messages.move_iter() {
self.message(message);
}
}
}

Expand Down
5 changes: 4 additions & 1 deletion src/librustc/driver/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,10 @@ pub fn build_session_(sopts: config::Options,
recursion_limit: Cell::new(64),
};

sess.lint_store.borrow_mut().register_builtin(Some(&sess));
{
sess.lint_store.borrow_mut().register_builtin(Some(&sess));
}

sess
}

Expand Down
5 changes: 2 additions & 3 deletions src/librustc/front/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,8 @@ impl<'a, 'v> Visitor<'v> for Context<'a> {
"unsafe_destructor") {
self.gate_feature("unsafe_destructor",
i.span,
"`#[unsafe_destructor]` allows too \
many unsafe patterns and may be \
removed in the future");
"`#[unsafe_destructor]` does nothing \
anymore")
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/librustc/metadata/tydecode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,9 @@ fn parse_region(st: &mut PState, conv: conv_did) -> ty::Region {
't' => {
ty::ReStatic
}
'n' => {
ty::ReFunction
}
'e' => {
ty::ReStatic
}
Expand Down
3 changes: 3 additions & 0 deletions src/librustc/metadata/tyencode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ pub fn enc_region(w: &mut SeekableMemWriter, cx: &ctxt, r: ty::Region) {
ty::ReScope(nid) => {
mywrite!(w, "s{}|", nid);
}
ty::ReFunction => {
mywrite!(w, "n");
}
ty::ReStatic => {
mywrite!(w, "t");
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/astencode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ impl tr for ty::Region {
ty::ReScope(id) => {
ty::ReScope(dcx.tr_id(id))
}
ty::ReEmpty | ty::ReStatic | ty::ReInfer(..) => {
ty::ReEmpty | ty::ReStatic | ty::ReInfer(..) | ty::ReFunction => {
*self
}
ty::ReFree(ref fr) => {
Expand Down
31 changes: 1 addition & 30 deletions src/librustc/middle/borrowck/gather_loans/lifetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,36 +163,7 @@ impl<'a, 'tcx> GuaranteeLifetimeContext<'a, 'tcx> {
// See the SCOPE(LV) function in doc.rs

match cmt.cat {
mc::cat_rvalue(temp_scope) => {
temp_scope
}
mc::cat_upvar(..) |
mc::cat_copied_upvar(_) => {
ty::ReScope(self.item_scope_id)
}
mc::cat_static_item => {
ty::ReStatic
}
mc::cat_local(local_id) |
mc::cat_arg(local_id) => {
ty::ReScope(self.bccx.tcx.region_maps.var_scope(local_id))
}
mc::cat_deref(_, _, mc::UnsafePtr(..)) => {
ty::ReStatic
}
mc::cat_deref(_, _, mc::BorrowedPtr(_, r)) |
mc::cat_deref(_, _, mc::Implicit(_, r)) => {
r
}
mc::cat_downcast(ref cmt) |
mc::cat_deref(ref cmt, _, mc::OwnedPtr) |
mc::cat_deref(ref cmt, _, mc::GcPtr) |
mc::cat_interior(ref cmt, _) |
mc::cat_discr(ref cmt, _) => {
self.scope(cmt)
}
}
mc::scope(self.bccx.tcx, cmt, self.item_scope_id)
}

fn report_error(&self, code: bckerr_code) {
Expand Down
6 changes: 6 additions & 0 deletions src/librustc/middle/borrowck/gather_loans/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,12 @@ impl<'a, 'tcx> GatherLoanCtxt<'a, 'tcx> {
ty::ReScope(id) => id,
ty::ReFree(ref fr) => fr.scope_id,

ty::ReFunction => {
// For purposes of the borrow check, we can consider
// this a loan for the outer function block.
self.item_ub
}

ty::ReStatic => {
// If we get here, an error must have been
// reported in
Expand Down
68 changes: 37 additions & 31 deletions src/librustc/middle/expr_use_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -511,37 +511,41 @@ impl<'d,'t,'tcx,TYPER:mc::Typer<'tcx>> ExprUseVisitor<'d,'t,TYPER> {
}
}
_ => {
let overloaded_call_type =
match self.tcx()
.method_map
.borrow()
.find(&MethodCall::expr(call.id)) {
Some(ref method_callee) => {
OverloadedCallType::from_method_origin(
self.tcx(),
&method_callee.origin)
}
None => {
self.tcx().sess.span_bug(
callee.span,
format!("unexpected callee type {}",
callee_ty.repr(self.tcx())).as_slice())
}
};
match overloaded_call_type {
FnMutOverloadedCall => {
self.borrow_expr(callee,
ty::ReScope(call.id),
ty::MutBorrow,
ClosureInvocation);
let overloaded_call_type;
{
let method_map = self.tcx().method_map.borrow();
overloaded_call_type = match method_map.find(
&MethodCall::expr(call.id)) {
Some(ref method_callee) => {
OverloadedCallType::from_method_origin(
self.tcx(),
&method_callee.origin)
}
None => {
self.tcx().sess.span_bug(
callee.span,
format!("unexpected callee type {}",
callee_ty.repr(self.tcx()))
.as_slice())
}
}
FnOverloadedCall => {
self.borrow_expr(callee,
ty::ReScope(call.id),
ty::ImmBorrow,
ClosureInvocation);
}
{
match overloaded_call_type {
FnMutOverloadedCall => {
self.borrow_expr(callee,
ty::ReScope(call.id),
ty::MutBorrow,
ClosureInvocation);
}
FnOverloadedCall => {
self.borrow_expr(callee,
ty::ReScope(call.id),
ty::ImmBorrow,
ClosureInvocation);
}
FnOnceOverloadedCall => self.consume_expr(callee),
}
FnOnceOverloadedCall => self.consume_expr(callee),
}
}
}
Expand Down Expand Up @@ -933,8 +937,10 @@ impl<'d,'t,'tcx,TYPER:mc::Typer<'tcx>> ExprUseVisitor<'d,'t,TYPER> {
// inferred by regionbk
let upvar_id = ty::UpvarId { var_id: id_var,
closure_expr_id: closure_expr.id };
let upvar_borrow = self.tcx().upvar_borrow_map.borrow()
.get_copy(&upvar_id);
let upvar_borrow = {
let upvar_borrow_map = self.tcx().upvar_borrow_map.borrow();
upvar_borrow_map.get_copy(&upvar_id)
};

self.delegate.borrow(closure_expr.id,
closure_expr.span,
Expand Down
69 changes: 0 additions & 69 deletions src/librustc/middle/kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,76 +67,7 @@ pub fn check_crate(tcx: &ty::ctxt) {
tcx.sess.abort_if_errors();
}

struct EmptySubstsFolder<'a, 'tcx: 'a> {
tcx: &'a ty::ctxt<'tcx>
}
impl<'a, 'tcx> ty_fold::TypeFolder<'tcx> for EmptySubstsFolder<'a, 'tcx> {
fn tcx<'a>(&'a self) -> &'a ty::ctxt<'tcx> {
self.tcx
}
fn fold_substs(&mut self, _: &subst::Substs) -> subst::Substs {
subst::Substs::empty()
}
}

fn check_struct_safe_for_destructor(cx: &mut Context,
span: Span,
struct_did: DefId) {
let struct_tpt = ty::lookup_item_type(cx.tcx, struct_did);
if !struct_tpt.generics.has_type_params(subst::TypeSpace)
&& !struct_tpt.generics.has_region_params(subst::TypeSpace) {
let mut folder = EmptySubstsFolder { tcx: cx.tcx };
if !ty::type_is_sendable(cx.tcx, struct_tpt.ty.fold_with(&mut folder)) {
span_err!(cx.tcx.sess, span, E0125,
"cannot implement a destructor on a \
structure or enumeration that does not satisfy Send");
span_note!(cx.tcx.sess, span,
"use \"#[unsafe_destructor]\" on the implementation \
to force the compiler to allow this");
}
} else {
span_err!(cx.tcx.sess, span, E0141,
"cannot implement a destructor on a structure \
with type parameters");
span_note!(cx.tcx.sess, span,
"use \"#[unsafe_destructor]\" on the implementation \
to force the compiler to allow this");
}
}

fn check_impl_of_trait(cx: &mut Context, it: &Item, trait_ref: &TraitRef, self_type: &Ty) {
let ast_trait_def = *cx.tcx.def_map.borrow()
.find(&trait_ref.ref_id)
.expect("trait ref not in def map!");
let trait_def_id = ast_trait_def.def_id();

// If this is a destructor, check kinds.
if cx.tcx.lang_items.drop_trait() == Some(trait_def_id) &&
!attr::contains_name(it.attrs.as_slice(), "unsafe_destructor")
{
match self_type.node {
TyPath(_, ref bounds, path_node_id) => {
assert!(bounds.is_none());
let struct_def = cx.tcx.def_map.borrow().get_copy(&path_node_id);
let struct_did = struct_def.def_id();
check_struct_safe_for_destructor(cx, self_type.span, struct_did);
}
_ => {
cx.tcx.sess.span_bug(self_type.span,
"the self type for the Drop trait impl is not a path");
}
}
}
}

fn check_item(cx: &mut Context, item: &Item) {
match item.node {
ItemImpl(_, Some(ref trait_ref), ref self_type, _) => {
check_impl_of_trait(cx, item, trait_ref, &**self_type);
}
_ => {}
}

visit::walk_item(cx, item)
}

Expand Down
Loading

0 comments on commit 1b9a16c

Please sign in to comment.