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

Borrowck regression: allows segfault in 1.27.1 #52213

Closed
NilSet opened this issue Jul 10, 2018 · 10 comments
Closed

Borrowck regression: allows segfault in 1.27.1 #52213

NilSet opened this issue Jul 10, 2018 · 10 comments
Assignees
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@NilSet
Copy link
Contributor

NilSet commented Jul 10, 2018

The following sample correctly fails to build on stable channel, but erroneously passes on beta and nightly.

It's worth noting that with NLL turned on it also correctly fails to build.

enum Inner {
    Stack {
        data: [u8;23]
    },
    Heap {
        capacity: usize,
        data: *mut u8
    }
}

struct SmallString {
    len: usize,
    inner: Inner
}

impl SmallString {
    fn push_str(&mut self, item: &str) {
        match (&mut self.inner, self.len + item.len()) {
            (Inner::Heap { capacity, ref data }, x) => {
                if x > *capacity {
                    self.grow();
                    // data is now null pointer
                }
                unsafe {
                    ::std::ptr::copy_nonoverlapping(item.as_ptr(), data.add(self.len), item.len())
                }
            },
            _ => ()
        }
    }
    fn grow(&mut self){
        // Invalidate borrowed Heap.data
        self.inner = Inner::Stack { data: [0;23] };
    }
}
@NilSet NilSet changed the title Borrowck regression: multiple mut borrows Borrowck regression: allows segfault in safe rust on beta Jul 10, 2018
@NilSet
Copy link
Contributor Author

NilSet commented Jul 10, 2018

Here is an example of being able to segfault in safe rust on beta

enum Inner {
    Stack {
        data: [u8;23]
    },
    Heap {
        data: Box<[u8]>
    }
}

struct SmallString {
    len: usize,
    inner: Inner
}

impl SmallString {
    fn push_str(&mut self, item: &str) {
        match (&mut self.inner, self.len + item.len()) {
            (Inner::Heap { data }, x) => {
                println!("{}", data.len());
                if x > data.len() {
                    self.grow();
                    // data is now garbage pointer
                }
                println!("{:?}", data);
            },
            _ => ()
        }
    }
    fn grow(&mut self){
        // Invalidate borrowed Heap.data
        self.inner = Inner::Stack { data: [1,0,1,0,1,0,1,0,1,0,1,0,1,0,1,0,1,0,1,0,1,0,1] };
    }
}

fn main (){
    let slice = "this is gonna go bad".to_owned().into_bytes().into_boxed_slice();
    let mut ss = SmallString { len: slice.len(), inner: Inner::Heap { data: slice } };
    ss.push_str(" right now");
}

http://play.rust-lang.org/?gist=e9ebe1460df7a817467ee017de9655dc&version=beta&mode=debug&edition=2015

@NilSet NilSet changed the title Borrowck regression: allows segfault in safe rust on beta Borrowck regression: allows segfault in 1.27.1 Jul 10, 2018
@NilSet
Copy link
Contributor Author

NilSet commented Jul 10, 2018

Turns out this regression is now also in the new patch release on stable!

@kennytm kennytm added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. labels Jul 10, 2018
@est31
Copy link
Member

est31 commented Jul 10, 2018

If this can be reproduced on 1.27.1 but can't be reproduced on 1.27.0 (@NilSet said that on IRC), I guess that this regression is caused by #51686 .

@kennytm
Copy link
Member

kennytm commented Jul 10, 2018

cc @nikomatsakis #51686.

@nagisa
Copy link
Member

nagisa commented Jul 10, 2018

@rust-lang/compiler the obligatory ping to all of you.

@arielb1
Copy link
Contributor

arielb1 commented Jul 10, 2018

Minified:

fn transmute_lifetime<'a, 'b, T>(t: &'a (T,)) -> &'b T {
    match (&t, ()) {
        ((t,), ()) => t,
    }
}

fn main() {
    let x = {
        let y = Box::new((42,));
        transmute_lifetime(&y)
    };

    println!("{}", x);
}

@arielb1
Copy link
Contributor

arielb1 commented Jul 10, 2018

Did anyone verify that this reproduces on 1.27.1? I am not sure

@arielb1
Copy link
Contributor

arielb1 commented Jul 10, 2018

And now I'm in the state of "how does this ever work", given that cat_pattern is returning Err "correctly".

@stephaneyfx
Copy link
Contributor

@arielb1: Your minified example compiles with 1.27.1, and so does the second example in this thread.

@pnkfelix pnkfelix self-assigned this Jul 10, 2018
@arielb1
Copy link
Contributor

arielb1 commented Jul 10, 2018

I have a fix up, will push it soon.

bors pushed a commit that referenced this issue Jul 11, 2018
This looks like a typo introduced in #51686.

Fixes #52213.
bors pushed a commit that referenced this issue Jul 11, 2018
bors added a commit that referenced this issue Jul 11, 2018
 use the adjusted type for cat_pattern in tuple patterns

This looks like a typo introduced in #51686.

Fixes #52213.

r? @pnkfelix

beta + stable nominating because regression + unsoundness.
pietroalbini pushed a commit to pietroalbini/rust that referenced this issue Jul 14, 2018
pietroalbini pushed a commit to pietroalbini/rust that referenced this issue Jul 14, 2018
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Jul 18, 2018
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Jul 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants