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

binding-less matches on borrowed data are incorrectly allowed #45045

Closed
arielb1 opened this issue Oct 5, 2017 · 8 comments

Comments

Projects
None yet
7 participants
@arielb1
Copy link
Contributor

commented Oct 5, 2017

This is actually an unsoundness in AST borrowck: AST borrowck doesn't check things that are matched on for conflicting borrows unless there are actually pattern bindings, e.g. this compiles:

enum Xyz {
    A,
    B,
}

fn main() {
    let mut e = Xyz::A;
    let f = &mut e;
    match e {
        Xyz::A => println!("a"),
        Xyz::B => println!("b"),
    };
    *f = Xyz::B;
}

That is unsound because of e.g. data races. For example, this code compiles and runs, and semi-reliably segfaults:

#![feature(test)]
use std::{thread, time};

extern crate crossbeam;
extern crate test;

enum Xyz<'a> {
    A(&'a usize),
    B(usize),
}

fn main() {
    let mut e = Xyz::A(&0);
    crossbeam::scope(|scope| {
        scope.spawn(|| {
            let now = time::Instant::now();
            let ten_millis = time::Duration::from_millis(50);
            while now.elapsed() < ten_millis {
                for _ in 0..1000000 {
                    e = Xyz::A(&0);
                    test::black_box(()); // compiler barrier
                    e = Xyz::B(0xaaaaaaaa);
                }
            }
        });
        let ten_millis = time::Duration::from_millis(10);
        thread::sleep(ten_millis);
        match e {
            Xyz::A(&0) => println!("a"),
            _ => println!("b"),
        };
    });
}
@bstrie

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2017

This is actually an unsoundness in AST borrowck

Does this imply that this is another bug on the pile of things fixed by MIR borrowck?

@arielb1

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2017

Does this imply that this is another bug on the pile of things fixed by MIR borrowck?

Yeah.

@nikomatsakis nikomatsakis added this to the NLL Future Compat Warnings milestone Jan 4, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2018

This still compiles with MIR borrowck, so I guess more work is needed.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

Is this same problem as #27282 ?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

It seems like the problem is that the discriminant statement is not considered an access. Here is the relevant portion of the MIR:

bb0: {                              
        StorageLive(_1);                 // bb0[0]: scope 0 at src/main.rs:9:9: 9:14
        _1 = Xyz::A;                     // bb0[1]: scope 0 at src/main.rs:9:17: 9:23
        StorageLive(_2);                 // bb0[2]: scope 1 at src/main.rs:10:9: 10:10
        _2 = &mut _1;                    // bb0[3]: scope 1 at src/main.rs:10:13: 10:19
        _4 = discriminant(_1);           // bb0[4]: scope 3 at src/main.rs:12:9: 12:15
        switchInt(move _4) -> [0isize: bb1, 1isize: bb2, otherwise: bb3]; // bb0[5]: scope 3 at src/main.rs:12:9: 12:15
    }
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

@pnkfelix points out that two-phase borrows may be the cause. This example gets an error:

#![feature(nll)]

enum Xyz {
    A,
    B,
}

fn main() {
    let mut e = Xyz::A;
    let f = &mut e;
    let g = f;
    match e {
        Xyz::A => println!("a"),
        Xyz::B => println!("b"),
    };
    *g = Xyz::B;
}

this gets the following output:

error[E0503]: cannot use `e` because it was mutably borrowed
  --> src/main.rs:13:9
   |
10 |     let f = &mut e;
   |             ------ borrow of `e` occurs here
...
13 |         Xyz::A => println!("a"),
   |         ^^^^^^ use of borrowed `e`
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

Copying @arielb1's segfaulting example and enabling nll mode also gives an error playpen

@arielb1

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2018

@nikomatsakis

That's indeed what I expect under 2-phase borrows.

@aidanhs aidanhs added the P-medium label Jan 16, 2018

@bors bors closed this in f9e1b9c Jan 17, 2018

@nikomatsakis nikomatsakis modified the milestones: NLL run-pass without ICEs, NLL soundness violations Jan 19, 2018

Centril added a commit to Centril/rust that referenced this issue Dec 16, 2018

Rollup merge of rust-lang#56790 - rust-lang:borrowck-niche-discrimina…
…nts, r=nikomatsakis

Make RValue::Discriminant a normal Shallow read

Enum layout optimizations mean that the discriminant of an enum may not be stored in a tag disjoint from the rest of the fields of the enum. Stop borrow checking as though they are.

Run with MIRI to see why this is needed: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=09a3236685a06b6096e2e2e3968b852c.

This issue exists with the lexical borrow checker as well (see rust-lang#45045) so migrate mode should prevent this from being immediately breaking.

r? @nikomatsakis

Fixes rust-lang#56797

Centril added a commit to Centril/rust that referenced this issue Dec 16, 2018

Rollup merge of rust-lang#56790 - rust-lang:borrowck-niche-discrimina…
…nts, r=nikomatsakis

Make RValue::Discriminant a normal Shallow read

Enum layout optimizations mean that the discriminant of an enum may not be stored in a tag disjoint from the rest of the fields of the enum. Stop borrow checking as though they are.

Run with MIRI to see why this is needed: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=09a3236685a06b6096e2e2e3968b852c.

This issue exists with the lexical borrow checker as well (see rust-lang#45045) so migrate mode should prevent this from being immediately breaking.

r? @nikomatsakis

Fixes rust-lang#56797
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.