Skip to content

Borrow checker allows slice lifetime to be extended #22779

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

Closed
AerialX opened this issue Feb 24, 2015 · 11 comments · Fixed by #24553
Closed

Borrow checker allows slice lifetime to be extended #22779

AerialX opened this issue Feb 24, 2015 · 11 comments · Fixed by #24553
Assignees
Milestone

Comments

@AerialX
Copy link

AerialX commented Feb 24, 2015

It's possible to create a slice from another slice with a lifetime longer than the original without use of the "unsafe" keyword. This allows for accessing memory after the original contents of a slice have been free'd, among other bad things.

trait Tr<'a, T> {
    fn renew<'b: 'a>(self) -> &'b mut [T];
}
impl<'a, T> Tr<'a, T> for &'a mut [T] {
    fn renew<'b: 'a>(self) -> &'b mut [T] where 'a: 'b {
        &mut self[..]
    }
}

See this code for an example.

@edwardw
Copy link
Contributor

edwardw commented Feb 26, 2015

If the trait method also has that where clause:

trait Tr<'a, T> {
    fn renew<'b: 'a>(self) -> &'b mut [T] where 'a: 'b;
}
...

then the full example code does report a "*v does not live long enough" error.

@pnkfelix
Copy link
Member

P-backcompat-lang; but probably 1.0 polish. (i.e. not clear that this has to be dealt with by the beta.)

@pnkfelix
Copy link
Member

cc @nikomatsakis

@pnkfelix pnkfelix added this to the 1.0 milestone Feb 26, 2015
@nikomatsakis
Copy link
Contributor

The bug here is that the where clause is accepted in the impl when it is not present in the trait.

@nikomatsakis
Copy link
Contributor

I think I know why this is the case. I think @jroesch and I planned to come back and fix this and totally forgot, in particular!

@nikomatsakis nikomatsakis self-assigned this Feb 28, 2015
@jroesch
Copy link
Member

jroesch commented Mar 2, 2015

@nikomatsakis I have a vague memory of us needing to loop back and take care of this. I don't have much free time, but I'm interested and will poke at this, should be simple enough to patch.

@jroesch
Copy link
Member

jroesch commented Mar 3, 2015

So I looked into this and it seems that the bug is subtle as it only effects bounds that involve constraints where 'a : 'b and 'b : 'a (meaning 'a == 'b).

If we look at a reduced test case with just an added bound it is rejected:

trait T {
    fn foo<'a, 'b, A>(self) -> A;
}

impl T for () {
    fn foo<'a, 'b, A>(self) -> A where 'a : 'b {}
}

fn main() {}

If we go back to a trait of the form above we can see the same kind of soundness bug:

trait T<'a> {
    fn foo<'b : 'a, A>(&'a self) -> A;
}

impl<'a> T<'a> for () {
    fn foo<'b : 'a, A>(&'a self) -> A where 'a : 'b { panic!("bleh") }
}

fn main() {}

Though if we drop the extra bound:

trait T<'a> {
    fn foo<'b, A>(&'a self) -> A;
}

impl<'a> T<'a> for () {
    fn foo<'b, A>(&'a self) -> A where 'a : 'b { panic!("bleh") }
}

fn main() {}

@jroesch
Copy link
Member

jroesch commented Mar 3, 2015

The error we do trigger when dropping the constraint is here: https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/compare_method.rs#L394. This code was slated to be removed once we delete ParamBounds.

@nikomatsakis it isn't clear to me why adding both 'a : 'b and 'b : 'a causes this not to be checked. I played with a couple other extraneous constraints on the implementation and those implementations are all rejected.

@edwardw
Copy link
Contributor

edwardw commented Mar 3, 2015

@jroesch, I think that we have TraitPredicate excess check built-in in fulfill.rs#L326, but do no such thing for RegionOutlives or TypeOutlives predicates, fulfill.rs#L360 and fulfill.rs#L374 respectively.

@jroesch
Copy link
Member

jroesch commented Mar 3, 2015

@edwardw so the weird thing about the code above is some excess bounds will cause it to trip and others won't. It probably has to do with early/late bound regions since I'm not sure what else would be causing that to trip.

I see what you are talking about now that I look, and it may be the fact that the predicates are attached to the inference context that is causing this problem. The above chunk of code (related to error 195) needs to be removed anyways so I might just delete it and chase down the ramifications.

@nikomatsakis
Copy link
Contributor

Ah, I see the problem. This relates to the way that we store the relations among free regions in a global table. This table winds up being populated by the impl's where clause and then used to justify the impl's where clause, which is broken. I'm going to see what I can do about moving that stuff out of a global table, which is fairly bogus.

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Apr 18, 2015
table, introduce a `FreeRegionMap` data structure. regionck computes the
`FreeRegionMap` for each fn and stores the result into the tcx so that
borrowck can use it (this could perhaps be refactored to have borrowck
recompute the map, but it's a bid tedious to recompute due to the
interaction of closures and free fns). The main reason to do this is
because of rust-lang#22779 -- using a global table was incorrect because when
validating impl method signatures, we want to use the free region
relationships from the *trait*, not the impl.

Fixes rust-lang#22779.
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Apr 18, 2015
table, introduce a `FreeRegionMap` data structure. regionck computes the
`FreeRegionMap` for each fn and stores the result into the tcx so that
borrowck can use it (this could perhaps be refactored to have borrowck
recompute the map, but it's a bid tedious to recompute due to the
interaction of closures and free fns). The main reason to do this is
because of rust-lang#22779 -- using a global table was incorrect because when
validating impl method signatures, we want to use the free region
relationships from the *trait*, not the impl.

Fixes rust-lang#22779.
bors added a commit that referenced this issue Apr 24, 2015
… r=pnkfelix

Rather than storing the relations between free-regions in a global
table, introduce a `FreeRegionMap` data structure. regionck computes the
`FreeRegionMap` for each fn and stores the result into the tcx so that
borrowck can use it (this could perhaps be refactored to have borrowck
recompute the map, but it's a bid tedious to recompute due to the
interaction of closures and free fns). The main reason to do this is
because of #22779 -- using a global table was incorrect because when
validating impl method signatures, we want to use the free region
relationships from the *trait*, not the impl.

Fixes #22779.
bors added a commit that referenced this issue Apr 24, 2015
… r=pnkfelix

Rather than storing the relations between free-regions in a global
table, introduce a `FreeRegionMap` data structure. regionck computes the
`FreeRegionMap` for each fn and stores the result into the tcx so that
borrowck can use it (this could perhaps be refactored to have borrowck
recompute the map, but it's a bid tedious to recompute due to the
interaction of closures and free fns). The main reason to do this is
because of #22779 -- using a global table was incorrect because when
validating impl method signatures, we want to use the free region
relationships from the *trait*, not the impl.

Fixes #22779.
pnkfelix pushed a commit to pnkfelix/rust that referenced this issue Apr 29, 2015
table, introduce a `FreeRegionMap` data structure. regionck computes the
`FreeRegionMap` for each fn and stores the result into the tcx so that
borrowck can use it (this could perhaps be refactored to have borrowck
recompute the map, but it's a bid tedious to recompute due to the
interaction of closures and free fns). The main reason to do this is
because of rust-lang#22779 -- using a global table was incorrect because when
validating impl method signatures, we want to use the free region
relationships from the *trait*, not the impl.

Fixes rust-lang#22779.

----

This is cherry-pick of commit 6dfeda7 from PR rust-lang#24553

Manually resolved conflicts in:
  src/librustc/lib.rs
  src/librustc/middle/infer/region_inference/mod.rs
(both conflicts were related to changes to file structure.)
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.

6 participants