Aliasing &mut hiding behind trait objects

[alexcrichton]

Looking at some cargo code today, I think that we have a soundness hole where we have multiple instances of a &mut pointer hiding behind trait objects:

• We have ids which are loaded into Box<Source + 'a> objects, and each Source holds a pointer to the &'a mut Config instance
• Elsewhere, we have a HashMap<SourceId, Box<Source + 'a>> which is stored in one structure.
• In the structure storing the SourceMap, we call the load method which is where we pass in the &mut Config (note that the config pointer is also stored in the structure).

What ends up happening is that each Box<Source + 'a> has a handle to the same &'a mut Config as well as the PackageRegistry having a handle on &'a mut Config. I don't think that's a sound thing to do!

I vaguely remember trying to reduce this awhile ago and was unable to, but I wanted to make sure that this was listed here!

Nominating as I think this may be a hole that needs plugging.

[alexcrichton]

cc @nikomatsakis

[pnkfelix]

P-backcompat-lang, 1.0.

"Minimal" test case:

struct Foo<'a> { a: &'a mut Option<Box<uint>> }

trait T {
    fn mutate(&mut self);
}
impl<'a> T for Foo<'a> {
    fn mutate(&mut self) {
        *(self.a) = None;
    }
}

struct TOption<'a> {
    v: Option<Box<T + 'a>>
}

impl<'a> TOption<'a> {
    fn set(&mut self, x: Box<T + 'a>) {
        self.v = Some(x);
    }
    fn mutate(&mut self) {
        self.v.as_mut().map(|t| t.mutate());
    }
}

struct Baz<'a> {
    b: TOption<'a>,
    c: &'a mut Option<Box<uint>>
}

impl<'a> Baz<'a> {
    fn x(&'a mut self) {
        (|| {
            let meh = box Foo { a: self.c };
            self.b.set(meh);
        })();
        let inner = self.c.as_ref().unwrap();
        self.b.mutate();
        println!("{}", inner);
    }
}

fn main() {
    let mut bar = Some(box 1u);
    let mut baz = Baz {
        b: TOption { v: None },
        c: &mut bar
    };
    baz.x();
}

Output:

playpen: application terminated abnormally with signal 11 (Segmentation fault)

Fun fact, removing the closure from x does indeed make the compiler reject the program:

http://is.gd/r5pNxw

<anon>:53:21: 53:27 error: cannot borrow `*self.c` as immutable because it is also borrowed as mutable
<anon>:53         let inner = self.c.get();
                              ^~~~~~
<anon>:51:30: 51:36 note: previous borrow of `*self.c` occurs here; the mutable borrow prevents subsequent moves, borrows, or modification of `*self.c` until the borrow ends
<anon>:51         let meh = create_foo(self.c);
                                       ^~~~~~
<anon>:56:6: 56:6 note: previous borrow ends here
<anon>:50     fn x(&'a mut self) {
...
<anon>:56     }
              ^
error: aborting due to previous error

[alexcrichton]

Whoa, thanks so much @jakub-! I should probably fix Cargo now...

[nikomatsakis]

cc me

[nikomatsakis]

well, I guess I'm already cc'd ;)

[nikomatsakis]

/me investigating -- I have a hunch as to what's going on here. It's not a closure problem, though: if you replace the (|| { ... }() with just {...} you get the same outcome

[nikomatsakis]

I think the bug here is actually in the variance analysis, which fails to account for lifetimes used only in trait references. Argh!

[nikomatsakis]

I'll push a fix for this, but it also gives me (even more) reason to rebase my variance branch, which would have detected this problem (since it makes bivariance illegal). I was hoping to do that today in between compiles anyhow.