Use After Free in safe code using Vec and HashMap

[Gankra]

use std::collections::HashMap;

#[deriving(Show)]
struct Foo<'a> {
    buf: Vec<String>,
    map: HashMap<uint, &'a str>
}

impl<'a> Foo<'a> {
    fn new() -> Foo<'a> {
        Foo { buf: Vec::new(), map: HashMap::new() }
    }
    fn insert(&'a mut self, s: String) {
        self.buf.push(s);
        match self.buf.last() {
            None => panic!(""),
            Some(x) => { self.map.insert(x.len(), x.as_slice()); }
        }
    }
    fn do_bad_stuff(&mut self) {
        self.buf.clear();
        self.buf.push("test".into_string());
    }
}
fn main() {
    let mut foo = Foo::new();
    foo.insert("bad".into_string());
    foo.insert("stuff".into_string());
    println!("{}", foo);
    foo.do_bad_stuff();
    println!("{}", foo);
}

Output:

Foo { buf: [bad, stuff], map: {5: stuff, 3: bad} }
Foo { buf: [test], map: {5: stuff, 3: tes} }

via

CC @nikomatsakis

[nikomatsakis]

The problem seems to be that we are constraining the pattern with a region that is too small. It should be the enclosing block. I'm trying to figure out just why it is doing that, the code looks not obviously wrong.

[abonander]

If you remove the 'a from fn insert(&'a mut self, s: String), you get a lifetime error; if you shadow 'a with fn <'a>insert(&'a mut self, s: String), you also get a lifetime error; both are on the call to self.buf.last().

[nikomatsakis]

I take it back, I am incorrect about the source of the bug. but certainly you should not be able to call foo.insert() twice, because the first call ought to boorrow foo for the remainder of the block. I haven't quite figured out why that is not happening yet.

[Gankra]

It wouldn't be unheard of for this to be a bug in either collection's unsafe handling.

[abonander]

I'm wondering why this isn't a segfault since the two strings those slices point to should have been dropped in Vec::clear().

[dnaq]

The bug I posted on reddit found it's way to the issue tracker, I haven't studied the issue further, but hopefully the problem is in unsafe-code from the collections library.

[TimNN]

I believe the following code highlights the same bug:

struct Bar<'a> {
    i: i32,
    j: Option<&'a i32>,
}

impl <'a> Bar<'a> {
    fn new() -> Bar<'a> {
        Bar { i: 0, j: None }
    }

    fn set(&'a mut self, i: i32) {
        self.i = i;
        self.j = Some(&self.i);
    }
}

fn main() {
    let mut bar = Bar::new();
    bar.set(1);
    let j: &i32 = bar.j.unwrap();
    println!("{}", j);
    bar.i = 2;
    println!("{}", j);
}

Note that although j in main() is not mutable, it still changes it's value.

[abonander]

@TimNN Same thing happens there, too. You can't shadow 'a for set(), nor omit it from &'a mut self. Both are lifetime errors. But it suggests the former: fn set<'a>(&'a mut self, i: i32)

Technically you shouldn't be able to do this at all.

[Gankra]

Tried it with a bunch of other collections, and it works. Probably isn't an unsafe issue.

[TimNN]

The problem is, I believe, that the rust compiler doesn't realise that due to the &self.i borrow in set(), which is saved inside self (and can as such be accessed at a later time), self is still borrowed after the function returns, and as such further mutations should be disallowed.

[arielb1]

This is an issue with autoderef (free functions and method on a newtype of &mut Bar do not exhibit it ​– i.e. this is an issue with main not set). Seems to be a related of an earlier issue I responded to.

[arielb1]

Seems to be similar to #18566 (which is fixed through).

[TimNN]

Rust 0.12.0 correctly reports an error: http://is.gd/VXLgyb.

[arielb1]

Minimal example (unsound is the problematic function):

#![feature(tuple_indexing)]

pub struct Bar<'a>(i32, &'a i32);

impl<'a> Bar<'a> {
    pub fn set(&'a mut self) {
        self.1 = &self.0;
    }
}

// if I use this instead of bar.set I get a borrowck error
//pub fn set<'a>(b: &'a mut Bar<'a>) {
//    b.1 = &b.0;
//}

pub fn unsound<'f>(bar: &'f mut Bar<'f>) -> &'f mut Bar<'f> {
    bar.set();
    bar
}

fn main() {
    let r = &0i32;
    let b = &mut Bar(0, r);
    let b = unsound(b);
    let new_r = &b.1;
    b.0 = 1;
    println!("{}", new_r);
}

[arielb1]

This seems like a regionck bug:

#![feature(tuple_indexing)]

pub struct Bar<'bar>(&'bar i32);

impl<'bari> Bar<'bari> {
    pub fn get_ref(&'bari mut self) -> &'bari mut &'bari i32 {
        &mut self.0
    }
}

pub fn unsound_2<'f, 'g>(bar: &'g mut Bar<'f>) -> &'g mut &'g i32 {
    bar.get_ref()
}

fn main() {
    let r = &0i32;
    let bar = &mut Bar(r);
    let mut v = 0;
    {
        let ir = unsound_2(bar);
        *ir = &v;
    }
    v = 1;
    println!("{}", bar.0);
}

[nikomatsakis]

This was introduced by HRTB, fix is in #19544

[brson]

P-backcompat-lang. 1.0 patch enqueued.