dropck combined with type erasure allows use after free

[rkjnsn]

While I was trying attempting (and failing) to implement pythonesque's suggestion of using dropck to ensure that cycles were not visible from the destructors of any of the contained objects (thus allowing cycles and a cycle collector without the possibility of deref ever failing), I came across a situation (explicitly including a type-erased box in the structure) that seemed like it really should cause the compiler to enforce what I wanted it to, and that it was being overly permissive.

I have now created a test case that shows that this can, indeed, be used to obtain incorrect behavior. Specifically, this code...

use std::cell::RefCell;

struct VecHolder {
    v: Vec<u32>,
}

impl Drop for VecHolder {
    fn drop(&mut self) {
        println!("Dropping Vec");
    }
}

struct Container<'a> {
    v: VecHolder,
    d: RefCell<Vec<Box<Drop+'a>>>,
}

impl<'a> Container<'a> {
    fn new() -> Container<'a> {
        Container{d: RefCell::new(Vec::new()), v: VecHolder{v: vec![42; 100]}}
    }

    fn store<T: 'a+Drop>(&'a self, val: T) {
        self.d.borrow_mut().push(Box::new(val));
    }
}

struct Test<'a> {
    test: &'a Container<'a>,
}

impl<'a> Drop for Test<'a> {
    fn drop(&mut self) {
        println!("Val from Vec: {}", self.test.v.v[30]);
    }
}

fn main() {
    let container = Container::new();
    let test = Test{test: &container};
    container.store(test);
}

will reliably output the following:

Dropping Vec
Val from Vec: 42

You can see it in action on the playpen.

[alexcrichton]

cc @pnkfelix, perhaps related to dropck?

[pythonesque]

You can get this behavior with traits other than Drop, too. If that's the issue, it feels like this problem should be resolvable by always treating boxed trait objects as have an explicit Drop implementation in dropck. You might be able to improve that to only Sized types, I can't remember whether unsized types can have custom destructors.

[huonw]

@pythonesque http://is.gd/18YV2M

struct Bar { _x: [u8] }

impl Drop for Bar {
    fn drop(&mut self) {
        println!("dropping")
    }
}

fn main() {
    let _x: Box<Bar> = unsafe { std::mem::transmute(Box::new([]) as Box<[u8]>) };
}

warning: Ignoring drop flag in destructor for Barbecause the struct is unsized. See issue#16758
dropping
playpen: application terminated abnormally with signal 4 (Illegal instruction)

(Clickable: #16758)

[pythonesque]

Actually... now that I think about it, it may be a different problem: it seems like dropck isn't treating Box<Foo+'a> as having the same lifetime as 'a. To expand on what I mean, the code below issues a compiler error correctly:

use std::cell::RefCell;

struct VecHolder {
    v: Vec<u32>,
}

impl Drop for VecHolder {
    fn drop(&mut self) {
        println!("Dropping Vec");
    }
}

trait Foo {}

impl<'a> Foo for Test<'a> {}

struct Container<'a> {
    v: VecHolder,
    d: RefCell<Vec<Box<Foo + 'a>>>,
}

impl<'a> Container<'a> {
    fn new() -> Container<'a> {
        Container{d: RefCell::new(Vec::new()), v: VecHolder{v: vec![42; 100]}}
    }

    fn store(&'a self, val: Box<Foo + 'a>) {
        self.d.borrow_mut().push(val);
    }
}

struct Test<'a> {
    test: &'a Container<'a>,
}

impl<'a> Drop for Test<'a> {
    fn drop(&mut self) {
        println!("Val from Vec: {}", self.test.v.v[30]);
    }
}

fn main() {
    let (container, test);
    container = Container::new();
    test = Test{test: &container};
    container.store(Box::new(test) as Box<Foo>);
}

The problem appears to be occuring because we don't actually have to write it like this (rooting things that reference each other in the same let binding) for Box<Trait>, while we do in most (hopefully all) other cyclic situations (e.g. if you change it to &'a Foo instead of Box<Foo + 'a>, rustc won't let you get away with this). So I think it may just be a missed case in the implementation, rather than a conceptual issue... perhaps something subtle like not accounting for the fact that Box has special move semantics?

[pnkfelix]

Yeah the implementation obviously ignored this note from the RFC:

(Note: When encountering a D of the form Box<Trait+'b>, we conservatively assume that such a type has a Drop implementation parametric in 'b.)

Will fix ASAP