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

Unsoundness in borrowck around deref and mutable refs #29053

Closed
apasel422 opened this Issue Oct 14, 2015 · 13 comments

Comments

Projects
None yet
@apasel422
Copy link
Member

apasel422 commented Oct 14, 2015

Stable and nightly happily compile the following:

fn main() {
    let x: &'static str = "x";

    {
        let y = "y".to_string();
        let ref mut x = &*x;
        *x = &*y;
    }

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

Running the program on my local Ubuntu machine yields:

thread '<main>' panicked at 'index 0 and/or 0 in `�` do not lie on character boundary', ../rust/src/libcore/str/mod.rs:1444

The playpen yields different results depending on whether debug or release mode is chosen.

Someone is welcome to retitle this issue more accurately.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Oct 14, 2015

Nominating for P-high.

@eefriedman

This comment has been minimized.

Copy link
Contributor

eefriedman commented Oct 14, 2015

The code isn't semantically unsound. In let ref mut x = &*x;, x is supposed to be a pointer to an unnamed temporary containing an &str. For example:

fn main() {
    let ref mut y = "x";
    *y = "y";
}

The problem is just that trans is taking illegal shortcuts.

@apasel422

This comment has been minimized.

Copy link
Member Author

apasel422 commented Oct 14, 2015

@eefriedman Along those lines, in this example:

fn main() {
    let x: &'static str = "x";

    {
        let z = &mut &*x;
        *z = "z";
    }

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

the code compiles, runs, and prints "z".

@eefriedman

This comment has been minimized.

Copy link
Contributor

eefriedman commented Oct 14, 2015

@apasel422 Yes, same bug.

@sfackler sfackler added the T-compiler label Oct 14, 2015

@woowe

This comment has been minimized.

Copy link

woowe commented Oct 15, 2015

The same bug is coming up for me when I use html5ever.

@nikomatsakis nikomatsakis self-assigned this Oct 15, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Oct 15, 2015

triage: P-high

My preferred fix would be to go through MIR, but this may be worth fixing another way.

@rust-highfive rust-highfive added P-high and removed I-nominated labels Oct 15, 2015

@arielb1 arielb1 added I-wrong and removed I-unsound 💥 labels Oct 15, 2015

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Nov 19, 2015

cc me

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Nov 19, 2015

triage: p-medium

Another one that will be fixed by MIR, but not soon. If we think this is higher priority than that we should go back to p-high, but we should make a plan to actually fix it.

@nrc nrc added P-medium A-mir and removed P-high labels Nov 19, 2015

@briansmith

This comment has been minimized.

Copy link

briansmith commented Nov 19, 2015

Another one that will be fixed by MIR, but not soon.

It would make Rust advocacy a lot easier if bugs like this one were fixed before MIR. The typechecker and borrow checker are the reasons many people would choose Rust over other languages. The rewrite of the compiler (MIR) is likely to cause induce bugs itself, so I think a lot of people would like to see a very good stable release before MIR, to allow for skipping the first few (at least) MIR-based releases.

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Nov 20, 2015

@briansmith

Variations of this bug were one of the primary motivations for MIR - the code that is supposed to be handling this part is horrible and we don't want to make changes to it out of fear of introducing new bugs. I think we will have MIR-based trans+borrowck at 1.8 if not before.

@futile

This comment has been minimized.

Copy link

futile commented Aug 7, 2016

This seems to be fixed on nightly (but still crashes on stable & beta): https://is.gd/KqgBhe

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Aug 8, 2016

Only thing left to do here is land a test for it.

@futile

This comment has been minimized.

Copy link

futile commented Aug 8, 2016

I can add that.

futile pushed a commit to futile/rust that referenced this issue Aug 8, 2016

steveklabnik added a commit to steveklabnik/rust that referenced this issue Aug 10, 2016

Rollup merge of rust-lang#35505 - futile:test_29053, r=nikomatsakis
Add test for issue rust-lang#29053

This PR adds a test for rust-lang#29053 (currently fails on stage 0, but works with stage 1, as it should).

Fixes rust-lang#29053

jonathandturner added a commit to jonathandturner/rust that referenced this issue Aug 10, 2016

Rollup merge of rust-lang#35505 - futile:test_29053, r=nikomatsakis
Add test for issue rust-lang#29053

This PR adds a test for rust-lang#29053 (currently fails on stage 0, but works with stage 1, as it should).

Fixes rust-lang#29053

jonathandturner added a commit to jonathandturner/rust that referenced this issue Aug 11, 2016

Rollup merge of rust-lang#35505 - futile:test_29053, r=nikomatsakis
Add test for issue rust-lang#29053

This PR adds a test for rust-lang#29053 (currently fails on stage 0, but works with stage 1, as it should).

Fixes rust-lang#29053

@bors bors closed this in #35505 Aug 12, 2016

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.