squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

auto merge of #17784 : bkoropoff/rust/issue-17780, r=pcwalton

Browse source 
This fixes a soundness problem where `Fn` unboxed closures can mutate free variables in the environment.
The following presently builds:

```rust
#![feature(unboxed_closures, overloaded_calls)]

fn main() {
    let mut x = 0u;
    let _f = |&:| x = 42;
}
```

However, this is equivalent to writing the following, which borrowck rightly rejects:

```rust
struct F<'a> {
    x: &'a mut uint
}

impl<'a> Fn<(),()> for F<'a> {
    #[rust_call_abi_hack]
    fn call(&self, _: ()) {
        *self.x = 42; // error: cannot assign to data in a `&` reference
    }
}

fn main() {
    let mut x = 0u;
    let _f = F { x: &mut x };
}
```

This problem is unique to unboxed closures; boxed closures cannot be invoked through an immutable reference and are not subject to it.

This change marks upvars of `Fn` unboxed closures as freely aliasable in mem_categorization, which causes borrowck to reject attempts to mutate or mutably borrow them.

@zwarich pointed out that even with this change, there are remaining soundness issues related to regionck (issue #17403).  This region issue affects boxed closures as well.

Closes issue #17780
This commit is contained in:
bors 2014-10-09 07:12:30 +00:00
commit 1b46b007d7
9 changed files with 135 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
src/librustc/middle/borrowck/check_loans.rs
View file

@ -854,6 +854,12 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
check_for_aliasability_violation(this, span, b.clone());
}
mc::cat_copied_upvar(mc::CopiedUpvar {
kind: mc::Unboxed(ty::FnUnboxedClosureKind), ..}) => {
// Prohibit writes to capture-by-move upvars in non-once closures
check_for_aliasability_violation(this, span, guarantor.clone());
}
_ => {}
}

13
src/librustc/middle/borrowck/gather_loans/gather_moves.rs
View file

@ -133,16 +133,15 @@ fn check_and_get_illegal_move_origin(bccx: &BorrowckCtxt,
mc::cat_deref(_, _, mc::BorrowedPtr(..)) |
mc::cat_deref(_, _, mc::Implicit(..)) |
mc::cat_deref(_, _, mc::UnsafePtr(..)) |
mc::cat_upvar(..) | mc::cat_static_item |
mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Many, .. }) => {
mc::cat_upvar(..) | mc::cat_static_item => {
Some(cmt.clone())
}
// Can move out of captured upvars only if the destination closure
// type is 'once'. 1-shot stack closures emit the copied_upvar form
// (see mem_categorization.rs).
mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Once, .. }) => {
None
mc::cat_copied_upvar(mc::CopiedUpvar { kind: kind, .. }) => {
match kind.onceness() {
ast::Once => None,
ast::Many => Some(cmt.clone())
}
}
mc::cat_rvalue(..) |

11
src/librustc/middle/borrowck/gather_loans/move_error.rs
View file

@ -115,8 +115,15 @@ fn report_cannot_move_out_of(bccx: &BorrowckCtxt, move_from: mc::cmt) {
mc::cat_deref(_, _, mc::BorrowedPtr(..)) |
mc::cat_deref(_, _, mc::Implicit(..)) |
mc::cat_deref(_, _, mc::UnsafePtr(..)) |
mc::cat_upvar(..) | mc::cat_static_item |
mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Many, .. }) => {
mc::cat_upvar(..) | mc::cat_static_item => {
bccx.span_err(
move_from.span,
format!("cannot move out of {}",
bccx.cmt_to_string(&*move_from)).as_slice());
}
mc::cat_copied_upvar(mc::CopiedUpvar { kind: kind, .. })
if kind.onceness() == ast::Many => {
bccx.span_err(
move_from.span,
format!("cannot move out of {}",

2
src/librustc/middle/borrowck/gather_loans/restrictions.rs
View file

@ -72,7 +72,7 @@ impl<'a, 'tcx> RestrictionsContext<'a, 'tcx> {
SafeIf(lp.clone(), vec![lp])
}
mc::cat_upvar(upvar_id, _) => {
mc::cat_upvar(upvar_id, _, _) => {
// R-Variable, captured into closure
let lp = Rc::new(LpUpvar(upvar_id));
SafeIf(lp.clone(), vec![lp])

19
src/librustc/middle/borrowck/mod.rs
View file

@ -354,8 +354,12 @@ pub fn opt_loan_path(cmt: &mc::cmt) -> Option<Rc<LoanPath>> {
match cmt.cat {
mc::cat_rvalue(..) |
mc::cat_static_item |
mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Many, .. }) => {
mc::cat_static_item => {
None
}
mc::cat_copied_upvar(mc::CopiedUpvar { kind: kind, .. })
if kind.onceness() == ast::Many => {
None
}
@ -363,9 +367,9 @@ pub fn opt_loan_path(cmt: &mc::cmt) -> Option<Rc<LoanPath>> {
Some(Rc::new(LpVar(id)))
}
mc::cat_upvar(ty::UpvarId {var_id: id, closure_expr_id: proc_id}, _) |
mc::cat_upvar(ty::UpvarId {var_id: id, closure_expr_id: proc_id}, _, _) |
mc::cat_copied_upvar(mc::CopiedUpvar { upvar_id: id,
onceness: _,
kind: _,
capturing_proc: proc_id }) => {
let upvar_id = ty::UpvarId{ var_id: id, closure_expr_id: proc_id };
Some(Rc::new(LpUpvar(upvar_id)))
@ -724,6 +728,13 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
format!("{} in an aliasable location",
prefix).as_slice());
}
mc::AliasableClosure(id) => {
self.tcx.sess.span_err(span,
format!("{} in a free variable from an \
immutable unboxed closure", prefix).as_slice());
span_note!(self.tcx.sess, self.tcx.map.span(id),
"consider changing this closure to take self by mutable reference");
}
mc::AliasableStatic(..) |
mc::AliasableStaticMut(..) => {
self.tcx.sess.span_err(

60
src/librustc/middle/mem_categorization.rs
View file

@ -83,7 +83,8 @@ pub enum categorization {
cat_rvalue(ty::Region), // temporary val, argument is its scope
cat_static_item,
cat_copied_upvar(CopiedUpvar), // upvar copied into proc env
cat_upvar(ty::UpvarId, ty::UpvarBorrow), // by ref upvar from stack closure
cat_upvar(ty::UpvarId, ty::UpvarBorrow,
Option<ty::UnboxedClosureKind>), // by ref upvar from stack or unboxed closure
cat_local(ast::NodeId), // local variable
cat_deref(cmt, uint, PointerKind), // deref of a ptr
cat_interior(cmt, InteriorKind), // something interior: field, tuple, etc
@ -93,10 +94,27 @@ pub enum categorization {
// (*1) downcast is only required if the enum has more than one variant
}
#[deriving(Clone, PartialEq)]
pub enum CopiedUpvarKind {
Boxed(ast::Onceness),
Unboxed(ty::UnboxedClosureKind)
}
impl CopiedUpvarKind {
pub fn onceness(&self) -> ast::Onceness {
match *self {
Boxed(onceness) => onceness,
Unboxed(ty::FnUnboxedClosureKind) |
Unboxed(ty::FnMutUnboxedClosureKind) => ast::Many,
Unboxed(ty::FnOnceUnboxedClosureKind) => ast::Once
}
}
}
#[deriving(Clone, PartialEq)]
pub struct CopiedUpvar {
pub upvar_id: ast::NodeId,
pub onceness: ast::Onceness,
pub kind: CopiedUpvarKind,
pub capturing_proc: ast::NodeId,
}
@ -571,14 +589,14 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
};
if var_is_refd {
self.cat_upvar(id, span, var_id, fn_node_id)
self.cat_upvar(id, span, var_id, fn_node_id, None)
} else {
Ok(Rc::new(cmt_ {
id:id,
span:span,
cat:cat_copied_upvar(CopiedUpvar {
upvar_id: var_id,
onceness: closure_ty.onceness,
kind: Boxed(closure_ty.onceness),
capturing_proc: fn_node_id,
}),
mutbl: MutabilityCategory::from_local(self.tcx(), var_id),
@ -591,20 +609,15 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
.unboxed_closures()
.borrow();
let kind = unboxed_closures.get(&closure_id).kind;
let onceness = match kind {
ty::FnUnboxedClosureKind |
ty::FnMutUnboxedClosureKind => ast::Many,
ty::FnOnceUnboxedClosureKind => ast::Once,
};
if self.typer.capture_mode(fn_node_id) == ast::CaptureByRef {
self.cat_upvar(id, span, var_id, fn_node_id)
self.cat_upvar(id, span, var_id, fn_node_id, Some(kind))
} else {
Ok(Rc::new(cmt_ {
id: id,
span: span,
cat: cat_copied_upvar(CopiedUpvar {
upvar_id: var_id,
onceness: onceness,
kind: Unboxed(kind),
capturing_proc: fn_node_id,
}),
mutbl: MutabilityCategory::from_local(self.tcx(), var_id),
@ -638,7 +651,8 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
id: ast::NodeId,
span: Span,
var_id: ast::NodeId,
fn_node_id: ast::NodeId)
fn_node_id: ast::NodeId,
kind: Option<ty::UnboxedClosureKind>)
-> McResult<cmt> {
/*!
* Upvars through a closure are in fact indirect
@ -666,7 +680,7 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
let base_cmt = Rc::new(cmt_ {
id:id,
span:span,
cat:cat_upvar(upvar_id, upvar_borrow),
cat:cat_upvar(upvar_id, upvar_borrow, kind),
mutbl:McImmutable,
ty:upvar_ty,
});
@ -1233,6 +1247,7 @@ pub enum InteriorSafety {
pub enum AliasableReason {
AliasableBorrowed,
AliasableClosure(ast::NodeId), // Aliasable due to capture by unboxed closure expr
AliasableOther,
AliasableStatic(InteriorSafety),
AliasableStaticMut(InteriorSafety),
@ -1287,18 +1302,29 @@ impl cmt_ {
b.freely_aliasable(ctxt)
}
cat_copied_upvar(CopiedUpvar {onceness: ast::Once, ..}) |
cat_rvalue(..) |
cat_local(..) |
cat_upvar(..) |
cat_deref(_, _, UnsafePtr(..)) => { // yes, it's aliasable, but...
None
}
cat_copied_upvar(CopiedUpvar {onceness: ast::Many, ..}) => {
Some(AliasableOther)
cat_copied_upvar(CopiedUpvar {kind: kind, capturing_proc: id, ..}) => {
match kind {
Boxed(ast::Once) |
Unboxed(ty::FnOnceUnboxedClosureKind) |
Unboxed(ty::FnMutUnboxedClosureKind) => None,
Boxed(_) => Some(AliasableOther),
Unboxed(_) => Some(AliasableClosure(id))
}
}
cat_upvar(ty::UpvarId { closure_expr_id: id, .. }, _,
Some(ty::FnUnboxedClosureKind)) => {
Some(AliasableClosure(id))
}
cat_upvar(..) => None,
cat_static_item(..) => {
let int_safe = if ty::type_interior_is_unsafe(ctxt, self.ty) {
InteriorUnsafe

6
src/librustc/middle/typeck/check/regionck.rs
View file

@ -1552,7 +1552,7 @@ fn link_reborrowed_region(rcx: &Rcx,
// Detect references to an upvar `x`:
let cause = match ref_cmt.cat {
mc::cat_upvar(ref upvar_id, _) => {
mc::cat_upvar(ref upvar_id, _, _) => {
let mut upvar_borrow_map =
rcx.fcx.inh.upvar_borrow_map.borrow_mut();
match upvar_borrow_map.find_mut(upvar_id) {
@ -1686,7 +1686,7 @@ fn adjust_upvar_borrow_kind_for_mut(rcx: &Rcx,
mc::cat_deref(base, _, mc::BorrowedPtr(..)) |
mc::cat_deref(base, _, mc::Implicit(..)) => {
match base.cat {
mc::cat_upvar(ref upvar_id, _) => {
mc::cat_upvar(ref upvar_id, _, _) => {
// if this is an implicit deref of an
// upvar, then we need to modify the
// borrow_kind of the upvar to make sure it
@ -1739,7 +1739,7 @@ fn adjust_upvar_borrow_kind_for_unique(rcx: &Rcx, cmt: mc::cmt) {
mc::cat_deref(base, _, mc::BorrowedPtr(..)) |
mc::cat_deref(base, _, mc::Implicit(..)) => {
match base.cat {
mc::cat_upvar(ref upvar_id, _) => {
mc::cat_upvar(ref upvar_id, _, _) => {
// if this is an implicit deref of an
// upvar, then we need to modify the
// borrow_kind of the upvar to make sure it

50
src/test/compile-fail/issue-17780.rs Normal file
View file

@ -0,0 +1,50 @@
// Copyright 2014 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
#![feature(unboxed_closures, overloaded_calls)]
fn set(x: &mut uint) { *x = 5; }
fn main() {
// By-ref captures
{
let mut x = 0u;
let _f = |&:| x = 42;
//~^ ERROR cannot assign to data in a free
// variable from an immutable unboxed closure
let mut y = 0u;
let _g = |&:| set(&mut y);
//~^ ERROR cannot borrow data mutably in a free
// variable from an immutable unboxed closure
let mut z = 0u;
let _h = |&mut:| { set(&mut z); |&:| z = 42; };
//~^ ERROR cannot assign to data in a
// free variable from an immutable unboxed closure
}
// By-value captures
{
let mut x = 0u;
let _f = move |&:| x = 42;
//~^ ERROR cannot assign to data in a free
// variable from an immutable unboxed closure
let mut y = 0u;
let _g = move |&:| set(&mut y);
//~^ ERROR cannot borrow data mutably in a free
// variable from an immutable unboxed closure
let mut z = 0u;
let _h = move |&mut:| { set(&mut z); move |&:| z = 42; };
//~^ ERROR cannot assign to data in a free
// variable from an immutable unboxed closure
}
}

4
src/test/run-pass/unboxed-closures-by-ref.rs
View file

@ -28,8 +28,8 @@ fn main() {
let mut x = 0u;
let y = 2u;
call_fn(|&:| x += y);
call_fn(|&:| assert_eq!(x, 0));
call_fn_mut(|&mut:| x += y);
call_fn_once(|:| x += y);
assert_eq!(x, y * 3);
assert_eq!(x, y * 2);
}