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 · 24 comments · Fixed by #64221
Closed

Drop and leaking &mut references #31567

cuviper opened this issue Feb 11, 2016 · 24 comments · Fixed by #64221
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cuviper
Copy link
Member

cuviper 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
Copy link
Member

nagisa 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 Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness and removed I-wrong labels Feb 11, 2016
@arielb1
Copy link
Contributor

arielb1 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
Copy link
Member

bluss 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
Copy link
Contributor

arielb1 commented Feb 11, 2016

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

@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 11, 2016
@nikomatsakis
Copy link
Contributor

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
Copy link
Contributor

triage: P-high

@pnkfelix
Copy link
Member

pnkfelix 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
…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
…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
Copy link
Contributor

arielb1 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
Copy link
Member

@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
Copy link
Contributor

arielb1 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
Copy link
Contributor

@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
Copy link
Contributor

brson commented Jun 23, 2016

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

@brson brson added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Jun 23, 2016
@brson
Copy link
Contributor

brson commented Jun 23, 2016

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

@brson
Copy link
Contributor

brson commented Jun 23, 2016

Ah, felix is assigned. Ignore me.

@eddyb
Copy link
Member

eddyb 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
Copy link
Member Author

cuviper commented Jun 23, 2016

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

@brson brson removed the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Jun 23, 2016
@pnkfelix
Copy link
Member

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

@bors bors closed this as completed in 1e4e632 Mar 13, 2018
cramertj added a commit to cramertj/rust that referenced this issue Aug 2, 2018
…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
…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
…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.
@matthewjasper matthewjasper added the NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. label Sep 5, 2019
@matthewjasper
Copy link
Contributor

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

@matthewjasper matthewjasper reopened this Sep 5, 2019
Centril added a commit to Centril/rust that referenced this issue Sep 25, 2019
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
…hewjasper

 Rust 2015: No longer downgrade NLL errors

As per decision on a language team meeting as described in rust-lang#63565 (comment), in Rust 2015, we refuse to downgrade NLL errors, that AST borrowck accepts, into warnings and keep them as hard errors.

The remaining work to throw out AST borrowck and adjust some tests still remains after this PR.

Fixes rust-lang#38899
Fixes rust-lang#53432
Fixes rust-lang#45157
Fixes rust-lang#31567
Fixes rust-lang#27868
Fixes rust-lang#47366

r? @matthewjasper
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
…hewjasper

 Rust 2015: No longer downgrade NLL errors

As per decision on a language team meeting as described in rust-lang#63565 (comment), in Rust 2015, we refuse to downgrade NLL errors, that AST borrowck accepts, into warnings and keep them as hard errors.

The remaining work to throw out AST borrowck and adjust some tests still remains after this PR.

Fixes rust-lang#38899
Fixes rust-lang#53432
Fixes rust-lang#45157
Fixes rust-lang#31567
Fixes rust-lang#27868
Fixes rust-lang#47366

r? @matthewjasper
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
…hewjasper

 Rust 2015: No longer downgrade NLL errors

As per decision on a language team meeting as described in rust-lang#63565 (comment), in Rust 2015, we refuse to downgrade NLL errors, that AST borrowck accepts, into warnings and keep them as hard errors.

The remaining work to throw out AST borrowck and adjust some tests still remains after this PR.

Fixes rust-lang#38899
Fixes rust-lang#53432
Fixes rust-lang#45157
Fixes rust-lang#31567
Fixes rust-lang#27868
Fixes rust-lang#47366

r? @matthewjasper
@bors bors closed this as completed in 3b5fbb6 Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.