rustc_typeck::check::compare_method::compare_const_impl doesn't drain its fullfillment context nor invoke regionck.

[eddyb]

UPDATE: Mentoring instructions for fixing this bug can be found here.

---

There are some calls to add obligations to a fulfillment context but it's never checked.

Some examples of code that shouldn't compile:

trait TransmuteTarget: Sized {
    type Out: Sized;
}

// Can't use generic impl here.
impl TransmuteTarget for String {
    type Out = String;
}

trait Transmute: TransmuteTarget {
    const FROM: Self::Out;
}

impl<T: TransmuteTarget> Transmute for T {
    // Associated type projections aren't checked (obligation fulfillment).
    const FROM: &'static str = "foo";
}

trait Foo {
    const NAME: &'static str;
}

impl<'a> Foo for &'a () {
    // Lifetimes aren't checked (regionck).
    const NAME: &'a str = "unit";
}

cc @rust-lang/compiler

[nikomatsakis]

triage: P-medium

[withoutboats]

@eddyb does this block stabilization of associated consts?

[eddyb]

@withoutboats Yes, as it allows const types in impls that don't match the trait definition.

[withoutboats]

This can be exploited to elongate lifetimes:

#![feature(associated_consts)]

trait Evil<'a, T: 'static> {
    const EVIL: fn(&'a T) -> &'static T;
}

impl<'a, T: 'static> Evil<'a, T> for () {
    const EVIL: fn(&'a T) -> &'a T = identity;
}

fn identity<T: 'static>(s: &T) -> &T { s }

fn evil() -> &'static Box<i32> {
    let b = Box::new(0);
    <()>::EVIL(&b)
}


fn main() {
    println!("{}", **evil())
}

[aturon]

I'm renominating, given the exploit that @withoutboats found.

[nikomatsakis]

This is a fairly straight-forward fix, so I'm going to write up some mentoring instructions. Maybe somebody wants to jump in on it!

In general, the code in compare_method.rs is used to compare an associated item definition found in an impl against the trait specification. The name of the file is somewhat outdated; used to be that methods were the only thing where we needed special checks.

I actually suspect that the code for methods and constants could be combined into one function (as this comment suggests), but we can leave that for bonus points.

Let's walk through what the code does now. Imagine we have something like impl<T> Foo<T> for Vec<T> { const X: U = .... Our goal is to check that this type U matches what it says in the trait.

• First, we obtain the "trait reference" that the trait is implemented (Foo<T> for Vec<T>, in our example) here and extract its "substs" -- these are the values for the generic paramters of the trait ([Vec<T>, T], as the first thing is the Self parameter).
• Next, we load the type of the associated item from the impl (U) here
• Then we load the type from the trait here and apply the substitutions from the trait reference. This will substitute Vec<T> for Self and T for the trait's other type parameter, basically.
• We then have to normalize those types (remove any associated types we can). You always have to do this after loading types from the HIR currently. Here.
• Next we insist that these types are subtypes here and report an error if we get back an error result.

This is where things go wrong. First off, I suspect it'd be better to just require that the types are equal, rather than subtypes. That can be done by changing from sub to eq. But more importantly, the subtype operation works like this:

• If it gives back an Error, you get an error, that case is fine.
• If it gives back Ok, this does not mean you are done:
  • It also gives back a series of obligations that you must go and "prove" -- these might be trait requirements or other things. We are processing those obligations by "registering" them with the 'fulfillment context' here, which is good, but we're missing a step later.
  • Furthermore, there may be various region requirements setup that are being overlooked.

Basically, we've enqueued up a bunch of further checks that we ought to do, but nowhere are we doing them.

We ought to be adding two things (here I am giving links into the method code):

• "Selecting" all the obligations that we registered in the fulfillment context. This will check that all the trait obligations are met and so forth. The method code does that here -- this basically means calling select_all_or_error(&infcx).
• Performing the "region check" -- the method code does that here. This will do various region checks and then ensure that the region inference can succeed. I think this step is the one that is needed to prevent the example that @withoutboats gave.

In both cases, we should be able to essentially cut-and-paste the existing code.

[nikomatsakis]

Tagging as E-mentor.