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

borrowck is unsound in the presence of `&'static mut`s #27616

Closed
arielb1 opened this Issue Aug 9, 2015 · 17 comments

Comments

Projects
None yet
8 participants
@arielb1
Contributor

arielb1 commented Aug 9, 2015

STR

use std::mem;

fn leak<T>(mut b: Box<T>) -> &'static mut T {
    // isn't this supposed to be safe?
    let inner = &mut *b as *mut _;
    mem::forget(b);
    unsafe { &mut *inner }
}

fn evil(mut s: &'static mut String)
{
    // create alias
    let alias: &'static mut String = s;
    let inner: &str = &alias;
    // free value
    *s = String::new();
    let _spray = "0wned".to_owned();
    // ... and then use it
    println!("{}", inner);
}

fn main() {
    evil(leak(Box::new("hello".to_owned())));
}

Actual Result

memory corruption - 0wned is printed.

Thanks for alevy on IRC for reporting this.

@eddyb

This comment has been minimized.

Member

eddyb commented Aug 10, 2015

Maybe there should be a "strict outlives" requirement for reborrows for &mut T, effectively denying them for &'static mut T.
I wonder if that would break any code.

@alevy

This comment has been minimized.

Contributor

alevy commented Aug 10, 2015

I think there are two ways of looking at this issue:

  1. leak is not sound, because it mints a static reference to a value that is not actually statically allocate (i.e. it can be freed).
  2. leak is fine in principal, but aliasing 'static borrows is not -- the attack @arielb1 points out seems to rely on aliasing, and tricking the compiler into thinking it's safe to drop the string's Vec, although maybe it's not the only attack vector...

@eddyb's suggestions seems reasonable as a general rule, but in this case, isn't the borrow happening from an unsafe pointer, which doesn't have a lifetime? How would it prevent @arielb1's attack?

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Aug 10, 2015

@alevy

You can basically do that with every &'static mut pointer to a non-trivial type (you can alias them and then reborrow to a shorter lifetime).

It doesn't matter how you get them.

@alevy

This comment has been minimized.

Contributor

alevy commented Aug 10, 2015

@arielb1 Can you mount the attack on a variable that's actually statically allocated? i.e.:

static mut FOO : MyType = ...

Isn't this the reason static variables can't have Dropable types?

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Aug 10, 2015

Note that this would also make a thread_local that doesn't require Sync unsafe (because of RefCell).

@alevy

This comment has been minimized.

Contributor

alevy commented Aug 10, 2015

A counter example to my question/claim that @arielb1 came up with on IRC:

use std::cell::Cell;
use std::fmt::Debug;

fn transmute<X, Y: Debug>(a: Y, dummy: X, x: &'static mut Result<X, Y>,
                    y: &'static mut Result<X, Y>) -> &'static mut X {
    *x = Ok(dummy);
    let foo = x.as_mut().unwrap();
    *y = Err(a);
    return foo;
}

static mut FOO : Result<Option<&'static Cell<usize>>, usize> = Err(123);

fn main() {
    let baz : usize = 0xdeadbeef;

    let myfoo = unsafe { &mut FOO };

    let addr = &baz as *const usize as usize;
    let oops = transmute(addr, None, myfoo, myfoo);
    println!("Trying to print at address 0x{0:x}", addr);
    oops.map(|o| {
        println!("0x{0:x}", o.get());
        o.set(0xabcdef);
        println!("0x{0:x}", baz);
    });
}

So aliasing 'static references is just plain problematic, and clearly leak can't be done safely while aliasing is allowed. Does this raise a slightly broader question about whether 'static references should be aliasable from "safe" code? If aliasing 'static references is not allowed, can leak be safe?

@glaebhoerl

This comment has been minimized.

Contributor

glaebhoerl commented Aug 11, 2015

My intuition (and I suspect that of other people) was that global static mut items must be unsafe to access due to the combination of two reasons: (a) &mut references must always be unaliased, but (b) due to their global scope, it is impossible to enforce this for references created to static mut items. With a function like Box::leak(), however, (b) is not the case, so the usual move-only semantics of &mut should be sufficient to ensure uniqueness... except it's currently not due to the handling of reborrowing which this issue is about.

leak is not sound, because it mints a static reference to a value that is not actually statically allocate (i.e. it can be freed).

I believe &'static doesn't promise anything about how the referent was/is allocated, only that it will live forever, which is the case here. (It can't be freed according to the typesystem because there is no fn free(&'static T).)

@Aatch

This comment has been minimized.

Contributor

Aatch commented Aug 11, 2015

I agree with @eddyb that it's the re-borrow of s that's the issue. I guess that the 'static lifetime on alias is what is throwing it off. It really should be moved, not reborrowed, in this case, as the borrow doesn't seem to prevent usage of s like it does with a non-static lifetime. If you wrap the &'static mut String in some struct so re-borrowing doesn't happen, the code fails to compile as expected.

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Aug 11, 2015

@Aatch

There's no problem with the reborrowing - it's just that borrowck ignores the borrow part of the reborrow for some reason - probably this line.

If no reborrow happens, the value is moved as it should.

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Aug 11, 2015

@glaebhoerl

There is no free(&'a mut T) for any 'a. It's just that 'static values are required to be valid for the rest of the program's execution.

@alevy

static mut is known to be unsafe. I am worried about using some kind of Mutex/RefCell/IVar turning a &'static Mutex<T> into a &'static mut T.

@glaebhoerl

This comment has been minimized.

Contributor

glaebhoerl commented Aug 11, 2015

@arielb1 I know. I was trying to respond to @alevy's suspicion that the problem lies somewhere around the (purported) fact that these &'static mut values obtained by leak()ing "can be freed", unlike "normal" ones which are actually references to static items somewhere in the program.

@eddyb

This comment has been minimized.

Member

eddyb commented Aug 11, 2015

@arielb1 My intuition was that the resulting 'static lifetime carries no information that could associate it with a borrow, unlike other kinds of lifetimes.

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Aug 11, 2015

@eddyb

You can always borrow for the outermost function's lifetime - all borrows must intersect it.

@alexcrichton alexcrichton added the T-lang label Aug 12, 2015

@nikomatsakis nikomatsakis self-assigned this Aug 13, 2015

@huonw

This comment has been minimized.

Member

huonw commented Aug 13, 2015

A example of the core problem is:

fn foo(x: &'static mut i32) -> (&'static mut i32, &'static mut i32) {
    (x, x)
}

i.e. &'static muts can be aliased.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Aug 20, 2015

So the reason for this is clear enough. There is this code in gather_loans/mod.rs:

                    ty::ReStatic => {
                        // If we get here, an error must have been
                        // reported in
                        // `lifetime::guarantee_lifetime()`, because
                        // the only legal ways to have a borrow with a
                        // static lifetime should not require
                        // restrictions. To avoid reporting derived
                        // errors, we just return here without adding
                        // any loans.
                        return;
                    }

I think the assumption here was that &'static mut could only occur with a static mut. Anyway this is false, particularly now. Should be easy enough to fix I believe.

@eddyb

This comment has been minimized.

Member

eddyb commented Aug 20, 2015

@nikomatsakis I'm curious what the effects of removing that early return would be.
What would get tracked? Would the resulting restrictions be enough?

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Sep 9, 2015

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 9, 2015

@eddyb sorry for being slow to respond. The answer is that I think the resulting restrictions are enough, as long as we extend the loan out to the borders of the fn. I have a patch that does just this.

@bors bors closed this in #28321 Sep 10, 2015

jmesmon added a commit to jmesmon/leak that referenced this issue Feb 11, 2017

switch to returning mutable refs
fixes #2

With rust-lang/rust#27616 fixed, we're able to do this.

Not expected to break any users as &mut is coerced to & without issue.
Tests passed unchanged.

May want to consider adding tests to check for mutability.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment