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

trait cache can be poisoned through an associated type #42796

Closed
arielb1 opened this issue Jun 21, 2017 · 2 comments
Closed

trait cache can be poisoned through an associated type #42796

arielb1 opened this issue Jun 21, 2017 · 2 comments
Assignees
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@arielb1
Copy link
Contributor

arielb1 commented Jun 21, 2017

I was trying to root out the root cause of #39970 and I found this:

Meta

rustc 1.20.0-nightly (445077963 2017-06-20)

STR

pub trait Mirror<Smoke> {
    type Image;
}

impl<T, Smoke> Mirror<Smoke> for T {
    type Image = T;
}

pub fn poison<S>(victim: String) where <String as Mirror<S>>::Image: Copy {
    loop { drop(victim); }
}

fn main() {
    let s = "Hello!".to_owned();
    let mut s_copy = s;
    s_copy.push_str("World!");
    "0wned!".to_owned();
    println!("{}", s);
}

Expected result

Code does not compile, because it attempts to copy a string..

Actual result

Code compiles, is UB, generally prints 0wned!

Explanation

The global fulfilled predicates cache (GlobalFulfilledPredicates) tries to cache "environment-independent" predicates. The idea is that if a predicate contains neither parameters nor inference variables, then whether it holds is unaffected by the parameter environment.

This property almost holds - because in the absence of inference variables, trait impl search is "predicative", its results are independent of the universe of types, and therefore can be transferred from a "maximal universe" where the type parameters can be assigned variables.

However, that argument isn't 100% correct. If predicates are inconsistent, then there can be no such "maximal universe", and so String: Copy might be implied by <String as Mirror<T>>::Image: Copy.

We can say that this would be "fixed by Chalk" (as of course, in the absence of "intelligent" caching, this is not a problem), but that assumes the implementation of Chalk would not make the same mistake. I think a good way to fix this would be to check global predicates in the empty environment directly - if the current environment is consistent that will return the correct result, and if it is inconsistent we already don't always care about any kind of stability (e.g. we occasionally "know" that [T] is never Sized, even if some predicate tries to claim so).

cc @nikomatsakis @aturon @RalfJung (are you interested in trait systems?)

@arielb1 arielb1 added I-nominated I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 21, 2017
@nikomatsakis
Copy link
Contributor

triage: P-medium

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Jun 29, 2017
@nikomatsakis
Copy link
Contributor

Assigning medium priority. Obscure soundness bug. Fix will likely come soon in any case.

nikomatsakis pushed a commit to arielb1/rust that referenced this issue Jul 7, 2017
The evaluation cache already exists, and it can handle differing
parameter environments natively.

Fixes rust-lang#39970.
Fixes rust-lang#42796.
bors added a commit that referenced this issue Jul 7, 2017
Replace the global fulfillment cache with the evaluation cache

This uses the new "Chalk" ParamEnv refactoring to check "global" predicates in an empty environment, which should be correct because global predicates aren't affected by a consistent environment.

Fixes #39970.
Fixes #42796.

r? @nikomatsakis
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Mar 3, 2018
Maybe it has to do with the folding of obligations?
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Mar 8, 2018
Maybe it has to do with the folding of obligations?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants