Skip to content
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.

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

Don't allow Fn unboxed closures to mutate upvars #17784

Merged
merged 4 commits into from
Oct 9, 2014

Conversation

bkoropoff
Copy link
Contributor

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

#![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:

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

@huonw
Copy link
Member

huonw commented Oct 5, 2014

r? @nikomatsakis

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

Aren't by-value upvars also freely aliasable? E.g. you can have two concurrent calls to a given Fn closure by sharing it between threads in an Arc.

(I don't know if this is currently handled correctly or not.)

@bkoropoff
Copy link
Contributor Author

Hmm, it looked like that case was handled already:

            cat_copied_upvar(CopiedUpvar {onceness: ast::Many, ..}) => {
                Some(AliasableOther)
            }

However, it doesn't seem to prevent mutation. I'll have to look into it.

@bkoropoff
Copy link
Contributor Author

@huonw I have a fix for the by-value upvar case you mentioned. Should it get tacked on to this PR, or should I hold off and submit it separately after this one goes in?

Keep track of the kind of closure responsible for an upvar
This causes borrowck to correctly reject mutation or mutable borrows
of upvars in `Fn` unboxed closures since the closure environment is
aliasable.

This also tracks the responsible closure in the aliasability
information returned and uses it to give a helpful diagnostic.

Closes issue rust-lang#17780
@bkoropoff
Copy link
Contributor Author

It was cleaner to rebase and squash the fixes together. I think this is good to go.

bors added a commit that referenced this pull request Oct 9, 2014
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
@bors bors closed this Oct 9, 2014
@bors bors merged commit 4d2ff43 into rust-lang:master Oct 9, 2014
bors added a commit that referenced this pull request Oct 16, 2014
…sakis

This PR is based on #17784, which fixes closure soundness problems in borrowck.  Only the last two commits are unique to this PR.

My understanding of regionck is still evolving, so I'm not sure if this is the right approach.  Feedback is appreciated.

- In `link_reborrowed_region`, we account for the ability of upvars to
  change their mutability due to later processing.  A map of recursive
  region links we may want to establish in the future is maintained,
  with the links being established when the mutability of the borrow
  is adjusted.
- When asked to establish a region link for an upvar, we link it to
  the region of the closure body.  This creates the necessary
  constraint to stop unsound reborrows from the closure environment.

This partially (maybe completely) solves issue #17403.  Remaining work:

- This is only known to help with by-ref upvars.  I have not looked at
  by-value upvars yet to see if they can cause problems.
- The error diagnostics that result from failed region inference are
  pretty inscrutible.
bors added a commit that referenced this pull request Oct 17, 2014
…sakis

This PR is based on #17784, which fixes closure soundness problems in borrowck.  Only the last two commits are unique to this PR.

My understanding of regionck is still evolving, so I'm not sure if this is the right approach.  Feedback is appreciated.

- In `link_reborrowed_region`, we account for the ability of upvars to
  change their mutability due to later processing.  A map of recursive
  region links we may want to establish in the future is maintained,
  with the links being established when the mutability of the borrow
  is adjusted.
- When asked to establish a region link for an upvar, we link it to
  the region of the closure body.  This creates the necessary
  constraint to stop unsound reborrows from the closure environment.

This partially (maybe completely) solves issue #17403.  Remaining work:

- This is only known to help with by-ref upvars.  I have not looked at
  by-value upvars yet to see if they can cause problems.
- The error diagnostics that result from failed region inference are
  pretty inscrutible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants