Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
cuviper opened this issue Feb 11, 2016 · 23 comments

Comments

@cuviper
Copy link
Member

commented Feb 11, 2016

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

@nagisa

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

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.

@arielb1 arielb1 added I-unsound 💥 and removed I-wrong labels Feb 11, 2016

@arielb1

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

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;
    }
}
@bluss

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

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);
}
@arielb1

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

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

@pnkfelix pnkfelix added the T-compiler label Feb 11, 2016

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

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.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

triage: P-high

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 25, 2016

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.

pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 26, 2016

Add new `bckerr_code` variant for my warning-cycled error for rust-la…
…ng#31567.

My new variant, for better or worse, is carrying data that needs the
`'tcx` lifetime, so I am threading that onto `bckerr_code`, for better
or worse.

One might reasonably aruge that the `BckError` already carries a
`mc::cmt<'tcx>`, but I claim that distinguishing the identified
destructor type is important for the quality of the presented error
message.

(I am also passing along the identified `owner` lvalue, which is
probably of more use for internal debugging and less for error
reporting, but we may still want to try to work it into the
user-facing message as well.)

pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 26, 2016

FIXME Checkpoint third prototype code for addressing issue rust-lang#…
…31567.

The main thing to fix is that there is too much text, too much code,
and too much ad-hoc unchecked reasoning. (Regarding the text, the
obvious fix is to recast the presentation of my comments into
something appropriate for the README.md file.)
@arielb1

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

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.

Surely you mean distinguish between a type that has a destructor and a type that may implement Drop? We already do that to forbid destructuring Drop structs.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 26, 2016

@arielb1 yes I just meant the more precise analysis needs to use the appropriate predicate (i.e. that the type implements Drop), on the appropriate collection of types (plural) according to the l-value expression being borrowed.

@arielb1

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

@pnkfelix

I think we can just make it that a MIR drop conflicts with a loan of an lvalue only if there is a destructor "upstream" of that lvalue and not behind a &-reference. There is also a lifetime.end conflict which does not look through &mut, but we already do that.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2016

@pnkfelix and I were talking. I think this can be cleanly integrated into regionck -- in fact, some of the rules that are enforced in borrowck (I suspect) ought to moved to regionck. I'm going to try and tinker a bit here.

@brson

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2016

@rust-lang/compiler can you assign someone to this P-high bug?

@brson

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2016

I tagged this a regression from a quick scan, but not sure.

@brson

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2016

Ah, felix is assigned. Ignore me.

@eddyb

This comment has been minimized.

Copy link
Member

commented Jun 23, 2016

@brson "stable", "nightly" or "version" do not appear in any comment here and the only mention of a regression is @pnkfelix discussing a potential solution.
IIUC, the problem here can be reproduced on any Rust version in existence, the needed checks were never there. I would untag as regression, or rather leave it to @pnkfelix since he's already on this.

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2016

I can reproduce this all the way back to 1.0.0, at least.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jul 7, 2016

I suspect I won't be able to address this myself in this cycle, so keeping this at P-high is not a great idea.

@arielb1 suggests we should let this be fixed by switching to MIR-based borrowck.

@arielb1

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2018

@stjepang

This errors correctly with -Z borrowck=mir without NLL, so this is an NLL issue - looks like what I reported as #47470

@nikomatsakis nikomatsakis modified the milestones: NLL Future Compat Warnings, NLL run-pass without ICEs, NLL soundness violations Jan 16, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

Fixed by #48411

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Feb 27, 2018

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Feb 28, 2018

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Mar 1, 2018

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Mar 1, 2018

@bors bors closed this in 1e4e632 Mar 13, 2018

cramertj added a commit to cramertj/rust that referenced this issue Aug 2, 2018

Rollup merge of rust-lang#52782 - pnkfelix:issue-45696-dangly-paths-f…
…or-box, r=eddyb

[NLL] Dangly paths for box

Special-case `Box` in `rustc_mir::borrow_check`.

Since we know dropping a box will not access any `&mut` or `&` references, it is safe to model its destructor as only touching the contents *owned* by the box.

----

There are three main things going on here:

1. The first main thing, this PR is fixing a bug in NLL where `rustc` previously would issue a diagnostic error in a case like this:
```rust
fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
```

such code was accepted by the AST-borrowck in the past, but NLL was rejecting it with the following message ([playground](https://play.rust-lang.org/?gist=13c5560f73bfb16d6dab3ceaad44c0f8&version=nightly&mode=release&edition=2015))
```
error[E0597]: `**x` does not live long enough
 --> src/main.rs:3:40
  |
3 | fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
  |                                        ^^^^^^^^ - `**x` dropped here while still borrowed
  |                                        |
  |                                        borrowed value does not live long enough
  |
note: borrowed value must be valid for the anonymous lifetime rust-lang#1 defined on the function body at 3:1...
 --> src/main.rs:3:1
  |
3 | fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error
```

2. The second main thing: The reason such code was previously rejected was because NLL (MIR-borrowck) incorporates a fix for issue rust-lang#31567, where it models a destructor's execution as potentially accessing any borrows held by the thing being destructed. The tests with `Scribble` model this, showing that the compiler now catches such unsoundness.

However, that fix for issue rust-lang#31567 is too strong, in that NLL (MIR-borrowck) includes `Box` as one of the types with a destructor that potentially accesses any borrows held by the box. This thus was the cause of the main remaining discrepancy between AST-borrowck and MIR-borrowck, as documented in issue rust-lang#45696, specifically in [the last example of this comment](rust-lang#45696 (comment)), which I have adapted into the `fn foo` shown above.

We did close issue rust-lang#45696 back in December of 2017, but AFAICT that example was not fixed by PR rust-lang#46268. (And we did not include a test, etc etc.)

This PR fixes that case, by trying to model the so-called `DerefPure` semantics of `Box<T>` when we traverse the type of the input to `visit_terminator_drop`.

3. The third main thing is that during a review of the first draft of this PR, @matthewjasper pointed out that the new traversal of `Box<T>` could cause the compiler to infinite loop. I have adjusted the PR to avoid this (by tracking what types we have previously seen), and added a much needed test of this somewhat odd scenario. (Its an odd scenario because the particular case only arises for things like `struct A(Box<A>);`, something which cannot be constructed in practice.)

Fix rust-lang#45696.

cramertj added a commit to cramertj/rust that referenced this issue Aug 2, 2018

Rollup merge of rust-lang#52782 - pnkfelix:issue-45696-dangly-paths-f…
…or-box, r=eddyb

[NLL] Dangly paths for box

Special-case `Box` in `rustc_mir::borrow_check`.

Since we know dropping a box will not access any `&mut` or `&` references, it is safe to model its destructor as only touching the contents *owned* by the box.

----

There are three main things going on here:

1. The first main thing, this PR is fixing a bug in NLL where `rustc` previously would issue a diagnostic error in a case like this:
```rust
fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
```

such code was accepted by the AST-borrowck in the past, but NLL was rejecting it with the following message ([playground](https://play.rust-lang.org/?gist=13c5560f73bfb16d6dab3ceaad44c0f8&version=nightly&mode=release&edition=2015))
```
error[E0597]: `**x` does not live long enough
 --> src/main.rs:3:40
  |
3 | fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
  |                                        ^^^^^^^^ - `**x` dropped here while still borrowed
  |                                        |
  |                                        borrowed value does not live long enough
  |
note: borrowed value must be valid for the anonymous lifetime rust-lang#1 defined on the function body at 3:1...
 --> src/main.rs:3:1
  |
3 | fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error
```

2. The second main thing: The reason such code was previously rejected was because NLL (MIR-borrowck) incorporates a fix for issue rust-lang#31567, where it models a destructor's execution as potentially accessing any borrows held by the thing being destructed. The tests with `Scribble` model this, showing that the compiler now catches such unsoundness.

However, that fix for issue rust-lang#31567 is too strong, in that NLL (MIR-borrowck) includes `Box` as one of the types with a destructor that potentially accesses any borrows held by the box. This thus was the cause of the main remaining discrepancy between AST-borrowck and MIR-borrowck, as documented in issue rust-lang#45696, specifically in [the last example of this comment](rust-lang#45696 (comment)), which I have adapted into the `fn foo` shown above.

We did close issue rust-lang#45696 back in December of 2017, but AFAICT that example was not fixed by PR rust-lang#46268. (And we did not include a test, etc etc.)

This PR fixes that case, by trying to model the so-called `DerefPure` semantics of `Box<T>` when we traverse the type of the input to `visit_terminator_drop`.

3. The third main thing is that during a review of the first draft of this PR, @matthewjasper pointed out that the new traversal of `Box<T>` could cause the compiler to infinite loop. I have adjusted the PR to avoid this (by tracking what types we have previously seen), and added a much needed test of this somewhat odd scenario. (Its an odd scenario because the particular case only arises for things like `struct A(Box<A>);`, something which cannot be constructed in practice.)

Fix rust-lang#45696.

bors added a commit that referenced this issue Aug 2, 2018

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

[NLL] Dangly paths for box

Special-case `Box` in `rustc_mir::borrow_check`.

Since we know dropping a box will not access any `&mut` or `&` references, it is safe to model its destructor as only touching the contents *owned* by the box.

----

There are three main things going on here:

1. The first main thing, this PR is fixing a bug in NLL where `rustc` previously would issue a diagnostic error in a case like this:
```rust
fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
```

such code was accepted by the AST-borrowck in the past, but NLL was rejecting it with the following message ([playground](https://play.rust-lang.org/?gist=13c5560f73bfb16d6dab3ceaad44c0f8&version=nightly&mode=release&edition=2015))
```
error[E0597]: `**x` does not live long enough
 --> src/main.rs:3:40
  |
3 | fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
  |                                        ^^^^^^^^ - `**x` dropped here while still borrowed
  |                                        |
  |                                        borrowed value does not live long enough
  |
note: borrowed value must be valid for the anonymous lifetime #1 defined on the function body at 3:1...
 --> src/main.rs:3:1
  |
3 | fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error
```

2. The second main thing: The reason such code was previously rejected was because NLL (MIR-borrowck) incorporates a fix for issue #31567, where it models a destructor's execution as potentially accessing any borrows held by the thing being destructed. The tests with `Scribble` model this, showing that the compiler now catches such unsoundness.

However, that fix for issue #31567 is too strong, in that NLL (MIR-borrowck) includes `Box` as one of the types with a destructor that potentially accesses any borrows held by the box. This thus was the cause of the main remaining discrepancy between AST-borrowck and MIR-borrowck, as documented in issue #45696, specifically in [the last example of this comment](#45696 (comment)), which I have adapted into the `fn foo` shown above.

We did close issue #45696 back in December of 2017, but AFAICT that example was not fixed by PR #46268. (And we did not include a test, etc etc.)

This PR fixes that case, by trying to model the so-called `DerefPure` semantics of `Box<T>` when we traverse the type of the input to `visit_terminator_drop`.

3. The third main thing is that during a review of the first draft of this PR, @matthewjasper pointed out that the new traversal of `Box<T>` could cause the compiler to infinite loop. I have adjusted the PR to avoid this (by tracking what types we have previously seen), and added a much needed test of this somewhat odd scenario. (Its an odd scenario because the particular case only arises for things like `struct A(Box<A>);`, something which cannot be constructed in practice.)

Fix #45696.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.