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

Drop allowed while active borrows still in scope #12223

Closed
Thiez opened this issue Feb 13, 2014 · 17 comments · Fixed by #15313
Closed

Drop allowed while active borrows still in scope #12223

Thiez opened this issue Feb 13, 2014 · 17 comments · Fixed by #15313
Labels
A-lifetimes Area: Lifetimes / regions
Milestone

Comments

@Thiez
Copy link
Contributor

Thiez commented Feb 13, 2014

Updated bug

The compiler allows this code when it shouldn't

fn main() {
    let a = ~"";
    let b: ~[&str] = a.lines().collect();
    drop(a);
    for s in b.iter() {
        println!("{}", *s);
    }
}

Original description

The following code is buggy:

let foo = ~"hello";
let foo: ~[&str] = foo.words().collect();
let invalid_string = foo[0];

Invalid string contains, as the name suggests, an invalid string; sometimes I get a string filled with \x00, sometimes just garbage, and usually it will eventually assert that the string contains invalid characters.

It appears that the first foo gets dropped when the second one is assigned?
Either the first foo is supposed to be dropped as happens now, and the borrow checker should forbid this code (since the old foo no longer exists, &str cannot have a lifetime), or it should let the first foo live until the end of the current scope, and make the above work.

@alexcrichton
Copy link
Member

What platform are you running this on? This code compiles and prints hello for me on OSX

fn main() {
    let foo = ~"hello";
    let foo: ~[&str] = foo.words().collect();
    let invalid_string = foo[0];
    println!("{}", invalid_string);
}

@Thiez
Copy link
Contributor Author

Thiez commented Feb 13, 2014

I hadn't actually tried in my own rustc, I created the minimized version on rusti:

let foo = ~"hello"; let foo: ~[&str] = foo.words().collect(); foo[0]

Inspired by an actual bug that I got after trying to read some stuff from a file. It seems what I posted originally does actually compile and work properly on my system as well, I guess I should have tested that...
I'll try and find and find a minimized example tomorrow.

@alexcrichton
Copy link
Member

rusti is getting old at this point, and I suspect that this was fixed by #12158

@Thiez
Copy link
Contributor Author

Thiez commented Feb 13, 2014

This one asserts (sometimes, it's random) for real on my system, most recent build of master (debian 64-bit, rustc 0.10-pre 58eeb07 2014-02-12 14:51:48 -0800)

use std::io::stdio::{stdin};

fn main() {
  let contents = stdin().read_to_str();
  std::io::println(contents.clone().unwrap());
  let contents: ~[&str] = contents.unwrap().lines().collect();
  for row in contents.iter() {
    println!("Row: {:?}",*row);
  }
}

Assuming the executable is named bug: run with echo hello | ./bug
Removing the line that says std::io::println(contents.clone().unwrap()); gives me the output:

Row: "\x00\x04\x00\x00\x00"

Otherwise it appears random, usually asserting:

task '<main>' failed at 'assertion failed: (w != 0)', /home/thiez/sources/rust/src/libstd/str.rs:2504

@nikomatsakis
Copy link
Contributor

Quite plausibly due to #5781, though I haven't dug in in detail.

@Thiez
Copy link
Contributor Author

Thiez commented Feb 14, 2014

It seems the bug persists even without the shadowing so perhaps this is related to #10683, except for the 'match' part?

@alexcrichton
Copy link
Member

It appears that we allow this code to compile:

fn main() {
    let a = ~"";
    let b: ~[&str] = a.lines().collect();
    drop(a);
    for s in b.iter() {
        println!("{}", *s);
    }
}

Nominating, that's bad.

@Thiez
Copy link
Contributor Author

Thiez commented Feb 14, 2014

Any suggestions for a better name for this issue? The current name is misleading. Perhaps something along the lines of "Stale pointers can be created in safe code"?

@alexcrichton
Copy link
Member

Updated title and description

@pnkfelix
Copy link
Member

cc me

@flaper87
Copy link
Contributor

cc @flaper87

@alexcrichton
Copy link
Member

Interestingly enough, the compiler rejects this code:

fn main() {
    let a = ~"";
    let b = a.lines().to_owned_vec();
    drop(a);
    for s in b.iter() {
        println!("{}", *s);
    }
}

It appears that the type ascription is throwing off the compiler?

@flaper87
Copy link
Contributor

Another example taken from #12568

fn main() {
    let arr : ~[&str] = std::os::args()[1].split_str("::").collect();
    std::io::println("first " + arr[0]);
    std::io::println("first again " + arr[0]);
}

@pnkfelix
Copy link
Member

1.0, P-backcompat-lang

@pnkfelix pnkfelix added this to the 1.0 milestone Feb 27, 2014
@edwardw
Copy link
Contributor

edwardw commented Mar 10, 2014

The compiler correctly rejects:

fn main() {
    let a = ~"";
    let b: Vec<&str> = a.lines().collect();  // note: borrow of `a[..]` occurs here
    drop(a);    // error: cannot move out of `a` because it is borrowed
}

@nrc
Copy link
Member

nrc commented Mar 12, 2014

cc me

@edwardw
Copy link
Contributor

edwardw commented Mar 15, 2014

#12828 has taken a major detour to fix other two more fundamental bugs first. On top of that fix, the root cause of this bug is also becoming clearer.

Case 1:

let b: ~[&str] = a.lines().to_owned_vec();
drop(a);

Case 2:

let b: ~[&str] = a.lines().collect();
drop(a);

Case 3:

let b: Vec<&str> = a.lines().collect();
drop(a);

The difference here is that the RHS is directly assignable to LHS in case 1 but not in case 2 + 3 so rustc takes different code paths. And in latter two cases, there's bug in which upstream and downstream code inside rustc disagree with the order of two parameters so two regions are merged incorrectly. The fix is rather simple.

But the variance inference adds another twist to case 3. The result of variance inference can be used to do type parameter substitution, which is not enabled yet. The net effect is two negatives become positive; the current rustc rejects that too.It did compile on top of #12828, which in itself is incorrect.

So I guess the fix for this bug must wait for the PR 12828 to land.

bors added a commit that referenced this issue Jul 2, 2014
still in scope".

This issue was fixed by PR #12828 and #5781. All that was left was to
add tests.

Closes #12223.
bors added a commit that referenced this issue Jul 2, 2014
…crichton

with overloaded calls.

This enforces the mutability and borrow restrictions around overloaded
calls.

[breaking-change]

Closes #12223.

r? @alexcrichton
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 25, 2022
internal: Rename primeCaches config keys
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: Lifetimes / regions
Projects
None yet
7 participants