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

Lifetime bounds on structs do not entirely constrain impl fns #28609

Closed
talchas opened this Issue Sep 23, 2015 · 7 comments

Comments

Projects
None yet
5 participants
@talchas
Copy link

talchas commented Sep 23, 2015

struct Foo<'a, 'b: 'a>(&'a &'b i32);
impl<'a, 'b> Foo<'a, 'b> {
    fn foo(&self) {}
}

compiles. As far as I can tell, the bound on 'b is checked in the definitions of the functions, but isn't checked in the use of them. As a concrete example of this Kimundi/scoped-threadpool-rs#8 ( https://play.rust-lang.org/?gist=870da1405ea15e80f778&version=nightly ) will allow the incorrect use of ScopeRef in the test, while copying the bounds from the struct definition to the impl definition correctly rejects it.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Sep 24, 2015

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Sep 24, 2015

Minified:

struct S<'a, 'b: 'a>(Option<&'a &'b ()>);

impl<'a, 'b> S<'a, 'b> {
    fn xform(&self, a: &'b mut u32) -> &'a mut u32 {
        a
    }
}

fn return_dangling_pointer<'a>(s: S<'a, 'a>) -> &'a mut u32 {
    s.xform(&mut 3)
}

fn main() {
    let s = S(None);
    let a = return_dangling_pointer(s);
    println!("{}", a);
}
@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Sep 24, 2015

If you use a trait impl or UFCS instead of an inherent impl, the error is reported correctly.

@arielb1 arielb1 added the I-nominated label Sep 25, 2015

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Sep 25, 2015

This is a nasty soundness bug. I think its a just-a-bug I can fix.

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Sep 25, 2015

Can also be done via Deref

use std::ops::Deref;

struct S<'a, 'b: 'a>(Option<&'a &'b ()>, &'b u32);

impl<'a, 'b> Deref for S<'a, 'b> {
    type Target = &'a u32;
    fn deref(&self) -> &&'a u32 {
        &self.1
    }
}

fn return_dangling_pointer<'a>(s: S<'a, 'a>) -> &'a u32 {
    let four = 4;
    let mut s = s;
    s.1 = &four;
    &s // or &*s
}

fn main() {
    let temp = &42;
    let ptr = return_dangling_pointer(S(None,&temp));
    println!("{}", ptr);
}
@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Sep 25, 2015

Also via overloaded ops:

use std::ops::Shl;

struct S<'a, 'b: 'a>(Option<&'a &'b ()>);

impl<'a, 'b> Shl<&'b u32> for S<'a, 'b> {
    type Output = &'a u32;
    fn shl(self, t: &'b u32) -> &'a u32 { t }
}

fn return_dangling_pointer<'a>(s: S<'a, 'a>) -> &'a u32 {
    let s = s;
    s << &mut 3 /* avoid promotion */
}

fn main() {
    let a = return_dangling_pointer(S(None));
    println!("{}", a);
}

@arielb1 arielb1 added the T-compiler label Sep 25, 2015

arielb1 pushed a commit to arielb1/rust that referenced this issue Sep 26, 2015

Ariel Ben-Yehuda
ensure that the types of methods are well-formed
By RFC1214:
Before calling a fn, we check that its argument and return types are WF. This check takes place after all higher-ranked lifetimes have been instantiated. Checking the argument types ensures that the implied bounds due to argument types are correct. Checking the return type ensures that the resulting type of the call is WF.

The previous code only checked the trait-ref, which was not enough
in several cases.

As this is a soundness fix, it is a [breaking-change].

Fixes rust-lang#28609

arielb1 added a commit to arielb1/rust that referenced this issue Sep 26, 2015

ensure that the types of methods are well-formed
By RFC1214:
Before calling a fn, we check that its argument and return types are WF. This check takes place after all higher-ranked lifetimes have been instantiated. Checking the argument types ensures that the implied bounds due to argument types are correct. Checking the return type ensures that the resulting type of the call is WF.

The previous code only checked the trait-ref, which was not enough
in several cases.

As this is a soundness fix, it is a [breaking-change].

Fixes rust-lang#28609

arielb1 pushed a commit to arielb1/rust that referenced this issue Sep 29, 2015

Ariel Ben-Yehuda
ensure that the types of methods are well-formed
By RFC1214:
Before calling a fn, we check that its argument and return types are WF. This check takes place after all higher-ranked lifetimes have been instantiated. Checking the argument types ensures that the implied bounds due to argument types are correct. Checking the return type ensures that the resulting type of the call is WF.

The previous code only checked the trait-ref, which was not enough
in several cases.

As this is a soundness fix, it is a [breaking-change].

Fixes rust-lang#28609
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Oct 1, 2015

triage: P-high

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

arielb1 added a commit to arielb1/rust that referenced this issue Oct 2, 2015

ensure that the types of methods are well-formed
By RFC1214:
Before calling a fn, we check that its argument and return types are WF. This check takes place after all higher-ranked lifetimes have been instantiated. Checking the argument types ensures that the implied bounds due to argument types are correct. Checking the return type ensures that the resulting type of the call is WF.

The previous code only checked the trait-ref, which was not enough
in several cases.

As this is a soundness fix, it is a [breaking-change].

Fixes rust-lang#28609

bors added a commit that referenced this issue Oct 3, 2015

Auto merge of #28669 - arielb1:well-formed-methods, r=nikomatsakis
By RFC1214:
>    Before calling a fn, we check that its argument and return types are WF.
    
The previous code only checked the trait-ref, which was not enough
in several cases.
    
As this is a soundness fix, it is a [breaking-change]. Some new annotations are needed, which I think are because of #18653 and the imperfection of `projection_must_outlive` (that can probably be worked around by moving the wf obligation later).
    
Fixes #28609

r? @nikomatsakis

@bors bors closed this in #28669 Oct 3, 2015

@bluss bluss added the I-unsound 💥 label Oct 4, 2015

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.