Rust: Borrowck allows two mutable borrows when using match ergonomics

[Arnavion]

... which can be used to trigger a UAF in safe code.

rustc 1.26.0 (a77568041 2018-05-07)
binary: rustc
commit-hash: a7756804103447ea4e68a71ccf071e7ad8f7a03e
commit-date: 2018-05-07
host: x86_64-pc-windows-msvc
release: 1.26.0
LLVM version: 6.0

fn main() {
    let mut foo = Some("foo".to_string());
    let bar = &mut foo;
    match bar {
        Some(baz) => {
            bar.take();
            println!("{:?}", baz); // UAF, segfaults or prints garbage
        },
        None => unreachable!(),
    }
}

---

Original repro:

enum CacheFuture {
    Get(Option<String>)
}

fn main() {
    let mut f = CacheFuture::Get(Some("foo".to_string()));
    match &mut f {
        CacheFuture::Get(get) => match get {
            Some(mod_name) => {
                get.take();
                println!("{:?}", mod_name); // UAF, segfaults or prints garbage
            },
            None => unreachable!(),
        },
    }
}

---

Inserting explicit annotations to disable match ergonomics leads to borrowck correctly complaining about two mutable borrows, which leads me to believe this is only because of match ergonomics.

enum CacheFuture {
    Get(Option<String>)
}

fn main() {
    let mut f = CacheFuture::Get(Some("foo".to_string()));
    match *(&mut f) {
        CacheFuture::Get(ref mut get) => match *get {
            Some(ref mut mod_name) => {
                get.take();
                println!("{:?}", mod_name);
            },
            None => unreachable!(),
        },
    }
}

9  |             Some(ref mut mod_name) => {
   |                  ---------------- first mutable borrow occurs here
10 |                 get.take();
   |                 ^^^ second mutable borrow occurs here
...
14 |         },
   |         - first borrow ends here

[est31]

NLL fixes this. Adding #![feature(nll)] gives me this error:

error[E0499]: cannot borrow `*get` as mutable more than once at a time
  --> src/main.rs:12:17
   |
11 |             Some(mod_name) => {
   |                  -------- first mutable borrow occurs here
12 |                 get.take();
   |                 ^^^ second mutable borrow occurs here
13 |                 println!("{:?}", mod_name);
   |                                  -------- borrow later used here

[Arnavion]

#rust wasn't sure if this regression-from-stable-to-stable-worthy or not. On the one hand match ergonomics is new in 1.26, so it's not strictly a regression from 1.25 since the bad code would not have compiled there. On the other hand, 1.26 makes it easy to write code that hits this bug, so it would be nice to fix this in 1.26.2.

[Arnavion]

Now that 1.26.1 has been released, can someone confirm that this will be fixed in 1.26.2, or at least disable match ergonomics entirely? It isn't feasible to expect users to download a nightly and enable feature(nll) just to be sure they don't accidentally write code with this bug.

[novacrazy]

I for one would love the option to disable match ergonomics for crates. It's been nothing but distracting so far.

[pietroalbini]

Now that 1.26.1 has been released, can someone confirm that this will be fixed in 1.26.2, or at least disable match ergonomics entirely?

cc @Mark-Simulacrum

[Mark-Simulacrum]

I doubt that we will be disabling match ergonomics at this point due to their likely wide use and stabilization. It's possible we'd make another patch release but rather unlikely; we'll see what the results of compiler discussion are (I believe their meeting is Thursday). I do believe we'll backport the fix to beta so that it makes it for 1.27 if we have it by then.

[Arnavion]

It's possible we'd make another patch release but rather unlikely... I do believe we'll backport the fix to beta so that it makes it for 1.27 if we have it by then.

I figured compiling code to access garbage memory silently would be more serious than that. I guess users should stay with 1.25 then.

[aturon]

@novacrazy This issue isn't the place to argue about the feature itself (which has already gone through several open comment periods prior to stabilization). I'd encourage you to instead open a thread on https://internals.rust-lang.org/ to talk about your experiences in detail, if you think there's something actionable for the lang team.

[nikomatsakis]

This problem is in no way specific to match-default-bindings, apparently. The following program is also accepted (!):

fn main() {
    let mut foo = Some("foo".to_string());
    let bar = &mut foo;
    match bar {
        Some(ref mut baz) => {
            bar.take();
            drop(baz);
        },
        None => unreachable!(),
    }
}

[est31]

This problem is in no way specific to match-default-bindings, apparently. The following program is also accepted (!):

@nikomatsakis prepending &mut to the Some and None patterns in your example does give an error though, on both stable and nightly.

[Arnavion]

@nikomatsakis let bar = &mut foo; match bar { Some(...) is using match ergonomics. Without match ergonomics you would have had to write match *bar or &mut Some (edit: ... both of which correctly flag the double mutable borrow.)

[nikomatsakis]

I see, I see. Yes, of course. That helps! thanks.

[nikomatsakis]

Found the bug. "Implicit" derefs were accidentally being considered distinct from "explicit" derefs, leading borrowck to conclude that there are two distinct paths involved. Fix coming shortly.

[Mark-Simulacrum]

Core team just discussed this and we've decided to backport the fix and release 1.26.2.

[bstrie]

Can this be closed?

[Mark-Simulacrum]

Yes, I think so.

[pnkfelix]

Since this was fixed on AST-borrowck by #51235, removing NLL-fixed-by-NLL.