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

Fn unboxed closures can incorrectly mutate upvars #17780

Closed
bkoropoff opened this issue Oct 4, 2014 · 6 comments
Closed

Fn unboxed closures can incorrectly mutate upvars #17780

bkoropoff opened this issue Oct 4, 2014 · 6 comments

Comments

@bkoropoff
Copy link
Contributor

The following program will write to freed memory:

#![feature(unboxed_closures, overloaded_calls)]

fn main() {
    let mut v = vec![];
    let ref f = |&:| { v.push(0u); v.get_mut(0) };
    let x = (*f)();
    for _ in range(0u, 5) {
        (*f)();
    }
    *x = 42u;
}

Borrowck needs to consider upvars in capture-by-ref, self-by-ref unboxed closures to be aliasable so that mutation and mutable borrows are rejected. I'm working on a patch to do this.

@zwarich
Copy link

zwarich commented Oct 4, 2014

This looks like the unboxed closure version of #17403. The comment by @nikomatsakis there suggests that the correct fix is a bit deeper than just requiring the upvars to be aliasable.

@bkoropoff
Copy link
Contributor Author

I think there may be multiple issues. Right now unboxed closures permit the moral equivalent of mutating through an &&mut. It seems like borrowck should reject this out of hand regardless of the regions involved. I'm still new to the compiler, though, so I could be totally off here.

@zwarich
Copy link

zwarich commented Oct 5, 2014

@bkoropoff I meant that after your proposed fix there might be other soundness problems with unboxed closures, based on the fact that the inferred regions seem incorrect, but playing around for a bit I can't get any of them to happen.

@bkoropoff
Copy link
Contributor Author

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:

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 };
}

The key is that call takes self immutably, which means the closure can be invoked through an immutable reference and wreak havoc. Boxed closures can't be called through immutable references, so there isn't a problem here.

My patch fixes this, but it does not address the region issue. I'll retitle the bug to make it clear that it only covers this unboxed-closure-specific issue.

@bkoropoff bkoropoff changed the title By-reference upvar capture in unboxed closures is unsound Fn unboxed closures can incorrectly mutate upvars Oct 5, 2014
@zwarich
Copy link

zwarich commented Oct 5, 2014

A variation of this example causes an ICE: #17786

bkoropoff added a commit to bkoropoff/rust that referenced this issue Oct 5, 2014
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 added a commit to bkoropoff/rust that referenced this issue Oct 5, 2014
bors added a commit that referenced this issue 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
@zwarich
Copy link

zwarich commented Oct 9, 2014

#17784 is merged, so this is fixed.

@zwarich zwarich closed this as completed Oct 9, 2014
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

No branches or pull requests

2 participants