borrowck is unsound in the presence of &'static muts

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.

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.

@arielb1

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

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).
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?

@alevy

@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.

@arielb1

@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?

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

@arielb1

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?

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).)

@eddyb

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.

@Aatch

@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.

@glaebhoerl

@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.

@arielb1

@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.

@arielb1

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

@eddyb

@eddyb

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

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.

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.

@nikomatsakis

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

Treat loans of 'static data as extending to the end of the enclosing

@eddyb

@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.

Auto merge of #28321 - nikomatsakis:issue-27616, r=pnkfelix

switch to returning mutable refs

Navigation Menu

Repository navigation

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

STR

Actual Result

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

borrowck is unsound in the presence of &'static muts #27616

Description

STR

Actual Result

Activity

eddyb commented on Aug 10, 2015

alevy commented on Aug 10, 2015

arielb1 commented on Aug 10, 2015

alevy commented on Aug 10, 2015

arielb1 commented on Aug 10, 2015

alevy commented on Aug 10, 2015

glaebhoerl commented on Aug 11, 2015

Aatch commented on Aug 11, 2015

arielb1 commented on Aug 11, 2015

arielb1 commented on Aug 11, 2015

glaebhoerl commented on Aug 11, 2015

eddyb commented on Aug 11, 2015

arielb1 commented on Aug 11, 2015

huonw commented on Aug 13, 2015

nikomatsakis commented on Aug 20, 2015

eddyb commented on Aug 20, 2015

nikomatsakis commented on Sep 9, 2015

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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