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

Segfault in safe code: Index trait sugar v[i] ignores the borrow checker #15525

Closed
japaric opened this issue Jul 8, 2014 · 6 comments · Fixed by #15656
Closed

Segfault in safe code: Index trait sugar v[i] ignores the borrow checker #15525

japaric opened this issue Jul 8, 2014 · 6 comments · Fixed by #15656
Milestone

Comments

@japaric
Copy link
Member

japaric commented Jul 8, 2014

STR:

struct MyVec<T> {
    data: Vec<T>,
}

impl<T> Index<uint, T> for MyVec<T> {
    fn index<'a>(&'a self, &i: &uint) -> &'a T {
        assert!(i < self.data.len());
        &self.data.as_slice()[i]
    }
}

fn main() {
    let v = MyVec { data: vec!(box 1i, box 2, box 3) };

    let ok: &Box<int> = v.index(&0);

    // Partial move while `v` is borrowed
    let bad: Box<int> = v[0];

    // Boom!
    println!("{}", good);
}

Output:

$ rustc foo.rs && ./foo
[1]    20073 segmentation fault (core dumped)  ./foo

GDB:

$ gdb ./foo
(..)
Program received signal SIGSEGV, Segmentation fault.
0x0000000000489c79 in fmt::num::int.fmt..Show::fmt::he5f803ef1f438abdOsJ ()
(..)

Version:

$ rustc --version
rustc 0.11.0 (e2e237afc1f9887699414c33332faa1efaba41ce 2014-07-08 09:56:41 +0000)

cc @pcwalton

@japaric japaric changed the title Segfault: v.index(&i) returns &T, but v[i] returns T Segfault in safe code: v.index(&i) returns &T, but its sugar v[i] returns (moves) T Jul 8, 2014
@alexcrichton
Copy link
Member

Nominating.

@pnkfelix
Copy link
Member

pnkfelix commented Jul 9, 2014

It may be of interest to compare against our behavior from version 0.10 on what I believe to be an analogous program (using ~[~int] as the vector v):

playpen

In particular, I want to point out that some comments in the issue description do not match my intuition based on prior behavior on vectors: it should not be surprising that v[0] has type Box<int> rather than &Box<int>, since that is how the index sugar behaves.

What is surprising is that this program compiles. The borrow checker in principle should be rejecting it. Presumably proper integration between the Index trait and the borrow checker remains to be implemented.

@japaric
Copy link
Member Author

japaric commented Jul 9, 2014

it should not be surprising that v[0] has type Box

The [] operator can be used on v because MyVec implements the Index trait, since the Index trait returns &T, how can v[i] ever return T?

I believe to be an analogous program (using ~[~int] as the vector v):

Was the [] operator usable on owned vectors ~[T] because they implemented the old Index trait, or was it because they were special cased? (Special cased like the current slices &[T], they don't implement the Index trait but you can use the [] operator on them)

@huonw
Copy link
Member

huonw commented Jul 9, 2014

v[i] is sugar for *v.index(&i), i.e. with the dereference. This should work fine for things like &v[i] (since this is just equivalent to &*v.index(&i) and that is equivalent to v.index(&i)), and should be rejected for things like let x = v[i];, i.e. when trying to move out (for types that aren't Copy), just like the explicit let x = *v.index(i); call would be rejected.

@japaric
Copy link
Member Author

japaric commented Jul 9, 2014

v[i] is sugar for *v.index(&i), i.e. with the dereference.

If that's the case, then this should not compile because that would be a partial move out of a borrowed vector. (as @pnkfelix pointed)

I was under the impression that v[i] (v being a slice) returned a reference, but I just checked that's not the case.

I'll update the issue description.

@japaric japaric changed the title Segfault in safe code: v.index(&i) returns &T, but its sugar v[i] returns (moves) T Segfault in safe code: Index trait sugar v[i] ignores the borrow checker Jul 9, 2014
@pnkfelix
Copy link
Member

Assigning P-backcompat-lang, 1.0 milestone.

@pnkfelix pnkfelix added this to the 1.0 milestone Jul 10, 2014
nrc added a commit to nrc/rust that referenced this issue Jul 14, 2014
bors added a commit that referenced this issue Jul 16, 2014
Closes #15525

The important bit of this are the changes from line 445 in mem_categorization.rs. Most of the other changes are about adding an Implicit PointerKind, and this is only necessary for getting a decent error message :-s An alternative would have been to add an implciti/explicit flag to cat_deref, which could be mostly ignored and so would mean much fewer changes. However, the implicit state would only be valid if the PointerKind was BorrowedPtr, so it felt like it ought to be another kind of PointerKind. I still don't know which is the better design.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants