Variables moved from in match guards are still accessible in other match arms.

[Toby-S]

The following code segfaults at runtime on stable, beta, and nightly:

fn main() {
    let x = 10;
    let s = Custom { val: Vec::new() };

    match x {
        10 if test(s) => println!("oops"),
        _ => println!("eek {:?}", s.val),
    }
}

#[derive(Debug)]
pub struct Custom {
    val: Vec<u8>,
}

impl Drop for Custom {
    fn drop(&mut self) { println!("Dropping"); }    
}

#[allow(unused_variables)]
fn test(s: Custom) -> bool { false }

[Toby-S]

Possibly aided by #3478?

[eddyb]

Shorter testcase:

fn main() {
    let s = String::new();
    let _s = match 0 {
        0 if { drop(s); false } => String::from("oops"),
        _ => {
            // This should trigger an error,
            // s could have been moved from.
            s
        }
    };
}

CFG for this testcase:

[eddyb]

cc @rust-lang/compiler

This would be solved by the MIR, right?
The way match guards would be desugared in the MIR, they would be potential ancestors of following match arms, and a MIR borrowck wouldn't be able to miss them.

[LLBlumire]

Bringing mine and @eddyb 's IRC conversation into here for readability.

The issue is that the path on the left in the image above does not chain into the path on the right, meaning that drop(s) can be followed by { s }.

This seems to be a configuration issue in https://github.com/rust-lang/rust/blob/master/src/librustc/middle/cfg/construct.rs#L429-L529

[arielb1]

Wait can you move in guards?

[Manishearth]

@arielb1 you shouldn't be able to

[Aatch]

Hmm, looks like I missed this case when I fixed a very similar issue (moving in multiple guard expressions). The issue is that the way the final guard is handled means that it acts as if it can't fail. Wiring up the contents of prev_guards to arm exit in the case where the arm has no guard and no bindings (including wildcard) should fix it. It should also do the same at the very end, adding edges from the remaining guards to the exit of the match expression.

@Manishearth I tried disallowing it, but there's some code around that moves in guards. Its also a breaking change.

[Aatch]

Ok, in trying to fix this I ran into an issue that is probably why I didn't handle this case last time. Borrows due to by-ref bindings in matches are scoped the match expression itself, meaning that something like:

match foo_opt {
  Some(ref foo) if cond => { ... }
  None => { foo_opt.val = bar; }
}

will complain that you can't assign to foo_opt.val because it's already borrowed (which is clearly not true). It's essentially the canonical example for the kind of thing SEME will solve, except worse.

[bstrie]

@eddyb How did you get rustc to spit out the CFG like that?

[Toby-S]

@bstrie rustc file.rs -Z unstable-options --unpretty flowgraph=main > out.dot

[Manishearth]

Oh, it's unpretty now. I knew it used to be --pretty and was wondering why it wasn't working.

[eddyb]

@bstrie What @Toby-S said, followed by dot -O -Tpng out.dot (I would do SVG instead, which tends to be nicer to browse, since you can easily Ctrl+F in it, but I don't know how to host it).

[nikomatsakis]

Key question is whether it's worth trying to fix this now, or just accelerate efforts to port borrowck to MIR. There are already a number of these sorts of issues that would be fixed by such a thing. @Aatch what efforts did you make to fix this?

[nikomatsakis]

triage: P-high

High because soundness but maybe we should downgrade to medium or otherwise try to solve via MIR.

[Aatch]

@nikomatsakis I tried to change the CFG to represent the control flow more accurately but ran into the issue I described above. I think there needs to be a change to the regions around match arms, but I'm less familiar with the code there and not sure what to change or how.

[nrc]

triage: P-medium

Since we're basically waiting for MIR to fix this, it's not really P-high

[pnkfelix]

(tagging with A-mir since we are waiting for MIR to fix this)

[KiChjang]

This is still relevant on the latest nightly.

[sp-1234]

Is there an ETA for this or something?
This is defeated memory safety in the safe Rust subset so I guess this must be the top importance…

[bstrie]

@sp-1234 Regarding an ETA, the plan seems to be that this will be fixed by the act of migrating the borrow-checker to later in the pipeline of compiler passes. This has been in the works for quite a while now and should be ready by the end of this year. This will also fix several other soundness bugs of the same nature.

[eddyb]

@bstrie "later"? Are you referring to MIR borrowck? That has nothing to do with "earlier" or "later" (in fact, at least until recently, borrowck has always ran second-to-last, just before lints, which probably still are the last thing in the analysis passes, before translation), but rather the underlying abstraction on which the the checking is performed - with MIR, complex language-level constructs do not require as complex of handling, sometimes even being subsumed by the generality of MIR dataflow.

[arielb1]

Turns out we need either NLL (the conditionally-canceled borrow part) or having differently-typed guard binding and body binding variables to make this work:

use std::cell::RefCell;

fn assign<'a, 'b>(x: &RefCell<Option<&'a &'b mut u32>>, y: &'a &'b mut u32) {
    *x.borrow_mut() = Some(y);
}

fn main() {
    let (mut t, mut ten);
    ten = 10;
    t = Some(&mut ten);
    unsound(&mut t);
}

fn unsound<'a>(opt: &'a mut Option<&'a mut u32>) -> Option<&'a mut u32> {
    let store: RefCell<Option<&&mut u32>> = RefCell::new(None);
    match *opt {
        #[cfg(segfault)]
        Some(ref mut x) if {
            // this (making `x` escape from the arm) should be disallowed
            // - `x` shouldn't be `&'a mut &'a mut u32` here
            assign(&store, x);
            false
        } => {
           None
        }
        #[cfg(not(segfault))]
        Some(ref mut x) if { false } => {
            // but just using `x` should be fine: `x` has the type `&'a mut &'a mut u32` here
            Some(x)
        }
        ref mut x => {
            *x = None;
            println!("{:?}", store.borrow());
            None
        }
    }
}

[pnkfelix]

The current status of MIR-borrowck indicates that we can address the first example and the second example. We also correctly reject the cfg(segfault) case in the third example; but it unfortunately also rejects the cfg(not(segfault)) case in that same third example.

My inclination is to land regression tests, alongside feature(nll), for the three cases that segfault under AST borrowck. We obviously can also deploy feature(nll) as long as it is opt-in while the cfg(not(segfault)) case continues to be rejected. At that time, we should file a new ticket to investigate extending MIR-borrowck to accept the cfg(not_segfault)) scenario outlined in the third example.

[nikomatsakis]

FWIW, testing with ariel's example, NLL + MIR borrowck seems to accept cfg(not(segfault)) and reject cfg(segfault).

[pnkfelix]

I'm increasing priority on this because we should make sure we address it and other related issues as part of migration to NLL.

[nikomatsakis]

This is fixed in MIR borrowck:

https://play.rust-lang.org/?gist=3e78648bb9001bd5a96f59d09457f13b&version=nightly

[nikomatsakis]

Added to #47366 -- this is basically E-needstest now

[nikomatsakis]

triage: P-medium

[Manishearth]

#47549

[Toby-S]

Awesome! Thanks to everyone who played a part in resolving this