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.