Drop and leaking &mut references

There's a way to leak a &mut member borrowed from a struct, and then manipulate that member from the Drop handler -- even while it is immutably borrowed!

struct VecWrapper<'a>(&'a mut Vec<u8>);

impl<'a> IntoIterator for VecWrapper<'a> {
    type Item = &'a u8;
    type IntoIter = std::slice::Iter<'a, u8>;

    fn into_iter(self) -> Self::IntoIter {
        self.0.iter()
    }
}

impl<'a> Drop for VecWrapper<'a> {
    fn drop(&mut self) {
        // aggressively free memory
        self.0.clear();
        self.0.shrink_to_fit();
    }
}

fn main() {
    let mut v = vec![1, 2, 3];
    for i in VecWrapper(&mut v) {
        let s = "foo".to_owned(); // reused allocation
        println!("{}!  {} {:#x} '{}'", s, i, i, *i as char);
    }
}

playground output:

foo!  102 0x66 'f'
foo!  111 0x6f 'o'
foo!  111 0x6f 'o'

So this drop frees the vec's buffer, then an unrelated string allocation reuses the same memory, and those new values come out of the vec::Iter.

I think if the member was immutable &, then this "leak" from into_iter() would be fine since they can both share that reference, and Drop can't mutate it. But &mut must be exclusive, of course...

The behaviour feels somewhat intuitive to me, but is certainly a memory safety issue. Nominating.

cc @rust-lang/lang, I think?

EDIT: affects MIR as well, without an obvious fix.
EDIT2: Also @rust-lang/libs, because this is caused by IntoIter::into_iter.

Minified:

struct VecWrapper<'a>(&'a mut S);

struct S(Box<u32>);

fn get_dangling<'a>(v: VecWrapper<'a>) -> &'a u32 {
    let s_inner: &'a S = &*v.0;
    &s_inner.0
}

impl<'a> Drop for VecWrapper<'a> {
    fn drop(&mut self) {
        *self.0 = S(Box::new(0));
    }
}

fn main() {
    let mut s = S(Box::new(11));
    let vw = VecWrapper(&mut s);
    let dangling = get_dangling(vw);
    println!("{}", dangling);
}

MIR borrowck would of course catch get_dangling borrowing a pointer and dropping its owner, given that the relevant MIR is

fn(arg0: VecWrapper<'a>) -> &'a u32 {
    let var0: VecWrapper<'a>; // v
    let var1: &'a S; // s_inner
    let mut tmp0: &'a S;
    let mut tmp1: ();
    let mut tmp2: &'a Box<u32>;

    bb0: {
        var0 = arg0;
        tmp0 = &(*(var0.0: &'a mut S)); // borrows *var0.0 for 'a
        var1 = &(*tmp0);
        tmp2 = &((*var1).0: Box<u32>);
        return = &(*(*tmp2));
        drop(var0) -> bb1; // inside 'a, drops var0
    }

    bb1: {
        return;
    }
}

Here's another minification (playground)

struct DropSet<'a>(&'a mut u8);

impl<'a> DropSet<'a> {
    fn get(self) -> &'a u8 {
        self.0
    }
}

impl<'a> Drop for DropSet<'a> {
    fn drop(&mut self) {
        *self.0 = 0;
    }
}

fn main() {
    let mut x = 64;
    let s = DropSet(&mut x);
    let r = s.get();
    println!("{}", r);
}

I suppose we can leave this to fix-by-MIR-borrowck.

The problem (I think) is the special case rule in the borrow checker concerning &mut borrows whose owners go out of scope. That always seemed a bit suspicious to me. It does not consider destructors. Shouldn't be too hard to fix.

triage: P-high

@nikomatsakis

so, just a quick note on some ways @nikomatsakis and I have talked about resolving this:

My first instinct was to try to change either ExprUseVisitor or borrowck to inject the effect of mutating any local binding of type carrying a destructor at the end of its scope.
- One might think of this as trying to re-create the effect of the drop calls that we already explicit emit in the MIR.
- (However, this may also be too conservative; in particular, it depends on what one infers to be the side-effects of a drop call -- see further discussion below)
After discussing my attempts to explore the previous approach(es) with @nikomatsakis , he explained what he was thinking of in earlier comment -- namely, there is code in borrowck that already ensures, for any borrow, that it does not outlive the lifetime of the so-called "guarantor" for the borrow.
- @nikomatsakis suggested exploring changing that code to account for types that carry destructors
- However, he also noted that the guarantor code currently breaks out of its loop as soon as it sees any reference, rather than continuing to recurse until finding the underlying owner.
- (So clearly it wasn't going to be a three-line diff; some investigation was required.)
I spent today exploring how to extract the underlying "base guarantor", as I am calling it, for a cmt, and then checking that against the region of the borrow.
- I was pretty happy; the stdlib and compiler source only had one regression caused by the new check
- Namely, this code for cell::RefMut::map
- As an explanation, the reason that my check is complaining about this code is that RefMut has an attached destructor (since its borrow field implements Drop), and the orig.value is going to extend the lifetime of the borrowed value past the lifetime of orig itself. So check reasons that the destructor for the RefMut might mutate orig.value, making the alias illegal.
- However, now that I've taken the time to explain this scenario, I have come to realize that this case is reflecting a weakness in my proposed analysis.
- In particular, RefMut itself does not carry an impl Drop. It is non-Copy, sure, but the only thing that its destructor can do is recursively invoke the destructor for each of its fields. And that means that the only actual code that runs is on the orig.borrow field -- it has no access to the orig.value field.
This seems at least superficially similar to some of the issues associated with dropck.
- In particular, a more precise analysis here would distinguish between a type that itself implements Drop versus a type who merely carries a field that implements Drop.
- Such increased precision would allow the above code from cell::RefMut to go through, and thus reduce the breakage injected by the proposed change.

Auto merge of #52782 - pnkfelix:issue-45696-dangly-paths-for-box, r=e…

Reopening for visibility (other fixed-by-NLL bugs are kept open)

Navigation Menu

Repository navigation

`Drop` and leaking `&mut` references #31567

50 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Drop and leaking &mut references #31567

Description

Activity

nagisa commented on Feb 11, 2016

arielb1 commented on Feb 11, 2016

bluss commented on Feb 11, 2016

arielb1 commented on Feb 11, 2016

nikomatsakis commented on Feb 11, 2016

nikomatsakis commented on Feb 11, 2016

pnkfelix commented on Apr 25, 2016

50 remaining items

matthewjasper commented on Sep 5, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`Drop` and leaking `&mut` references #31567