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
Implied bounds on nested references + variance = soundness hole #25860
Comments
|
What's going on here is that This assumption was thought to be valid because any nested reference type |
|
One solution is to be more aggressive about checking WFedness, but there are other options to consider. |
|
triage: P-high T-lang |
|
I've been working on some proposal(s) that address this bug (among others), so assigning to me. |
invariance, this restores soundness to implied bounds (I think). :) Fixes rust-lang#25860.
invariance, this restores soundness to implied bounds (I think). :) Fixes rust-lang#25860.
invariance, this restores soundness to implied bounds (I think). :) Fixes rust-lang#25860.
|
There is a related problem that doesn't require variance on argument types. The current codebase doesn't check that the expected argument type is well-formed, only the provided one, which is a subtype of the expected one. This is insufficient (but easily rectified). |
invariance, this restores soundness to implied bounds (I think). :) Fixes rust-lang#25860.
|
@nikomatsakis I believe you meant to close this. |
|
The work needed to close this has not yet landed. It's in the queue though, once we finish up rust-lang/rfcs#1214. |
|
Sorry, I saw the commit but didn't notice that it hadn't been merged. |
|
One interesting thing to note, in light of the new WF checks that have landed with the preliminary implementation of rust-lang/rfcs#1214, is that if we change the definition of fn foo<'a, 'b, T>(_: &'a &'b (), v: &'b T) -> &'a T where 'b: 'a { v }then it seems like the code is rejected properly. I am currently trying to puzzle through whether this kind of "switch from implicit to explicit", assuming it were done as a kind of desugaring by the compiler, if that would be effectively the same as "remove support for contravariance" from the language, or if it represents some other path... |
|
Here's a variation on the original example that retains explicit types (rather than resorting to fn foo<'a, 'b, T>(_false_witness: Option<&'a &'b ()>, v: &'b T) -> &'a T { v }
fn bad<'c, 'd, T>(x: &'c T) -> &'d T {
// below is using contravariance to assign `foo` to `f`,
// side-stepping the obligation to prove `'c: 'd`
// implicit in the original `fn foo`.
let f: fn(Option<&'d &'d ()>, &'c T) -> &'d T = foo;
f(None, x)
}
fn main() {
fn inner() -> &'static String {
bad(&format!("hello"))
}
let x = inner();
println!("x: {}", x);
} |
After some reflection, I think this does represent a (perhaps appropriately) weakened variation on "remove contravariance from the language" In particular, if we did the desugaring right (where implied lifetime bounds from a |
|
as an aside, part of me does think that it would be better if we also added some way to write the proposed implied fn bad<'a, 'b>(g: fn (&'a &'b i32) -> i32) {
let _f: fn(x: &'b &'b i32) -> i32 = g;
}under my imagined new system, the above assignment of But it might be nice if we could actually write: fn explicit_types<'a, 'b>(callback: fn (&'a i32, &'b i32) -> i32 where 'a: 'b) {
...
}(note that the |
|
I take it from your comments that removing or restricting the variance is actually considered as a solution to this? That's surprising me. I cannot see variance at fault here. The observation underlying the issue is that implicit bounds are not properly preserved under variance. The most obvious solution (for me, anyways) would be to make the implicit bounds explicit: If "fn foo<'a, 'b, T>(&'a &'b (), &'b T) -> &'a T" would be considered mere syntactic sugar for "fn foo<'a, 'b, T>(&'a &'b (), &'b T) -> &'a T where 'b: 'a, 'a: 'fn, T: 'b" (using |
I've been saying the similar things to @nikomatsakis But the first step for me was to encode the test case in a way that made made it apparent (at least to me) that contravariance is (part of) why we are seeing this happen. I think that #25860 (comment) is very much the same as what you are describing: 1. Ensure the implicit bounds are actually part of the |
Yes, that sounds like we're talking about the same idea. |
|
I do not consider the problem to be fundamentally about contravariance -- but I do consider removing contravariance from fn arguments to be a pragmatic way to solve the problem, at least in the short term. In one of the drafts for RFC rust-lang/rfcs#1214, I wrote the section pasted below, which I think is a good explanation of the problem as I understand it: Appendix: for/where, an alternative viewThe natural generalization of our current These where-clauses must be discharged before the fn can be called. They can also be discharged through subtyping, if no higher-ranked regions are involved: that is, there might be a typing rule that allows a where clause to be dropped from the type so long as it can be proven in the current environment (similarly, having fewer where clauses would be a subtype of having more.) You can view the current notion of implied bounds as being a more limited form of this formalism where the For example, if you had a function: Under this RFC, the type of this function is: Under the for/where scheme, the full type would be: Now, if we upcast this type to only accept static data as argument, the where clauses are unaffected: Viewed this way, we can see why the current fn types (in which one cannot write where clauses explicitly) are invariant: changing the argument types is fine, but it also changes the where clauses, and the new where clauses are not a superset of the old ones, so the subtyping relation does not hold. That is, if we write out the implicit where clauses that result implicitly, we can see why variance on fns causes problems: Clearly, this subtype relationship should not hold, because the where clauses in the subtype are not implied by the supertype. |
|
To make the point clearer, the principal part of the issue involves higher-ranked types. The rank 0 issue should not be that hard to solve (by requiring WF when instantiating a function - I am a bit surprised we don't do so already). An example of the problem path (I am intentionally making the problem binder separate from the problem where-clause-container): fn foo<'a, 'b, T>() -> fn(Option<&'a &'b ()>, &'b T) -> &'a T {
fn foo_inner<'a, 'b, T>(_witness: Option<&'a &'b ()>, v: &'b T) -> &'a T {
v
}
foo_inner
}
fn bad<'c, 'd, T>(x: &'c T) -> &'d T {
// instantiate `foo`
let foo1: for<'a, 'b> fn() -> fn(Option<&'a &'b ()>, &'b T) -> &'a T = foo;
// subtyping: instantiate `'b <- 'c`
let foo2: for<'a> fn() -> fn(Option<&'a &'c ()>, &'c T) -> &'a T = foo1;
// subtyping: contravariantly 'c becomes 'static
let foo3: for<'a> fn() -> fn(Option<&'a &'static ()>, &'c T) -> &'a T = foo2;
// subtyping: instantiate `'a <- 'd`
let foo4: fn() -> fn(Option<&'d &'static ()>, &'c T) -> &'d T = foo3;
// boom!
foo4()(None, x)
}
fn main() {
fn inner() -> &'static String {
bad(&format!("hello"))
}
let x = inner();
println!("x: {}", x);
} |
|
What is the current status of fixing this bug? Rust-haters use this hole to demonstrate that the language is not safe. |
|
@XX This one specifically? As someone with no background in programming language theory, I'm curious what makes this one so special. |
|
I don't think there was any progress, unfortunately. That said, haters gonna hate -- I don't think that is a healthy reason to do anything.^^ There are much better reasons.
As someone with background in PL theory, I don't know. ;) I assume it's just because it's one of the oldest still-unresolved soundness bugs. |
|
I have also seen code snippets exploiting this issue as a response for "safe rust guaranties..." claims more than once.
I guess this is true. Personally, I also think that having this issue open for five years is kind of disturbing. |
|
Bumping an issue, since it was found (link to the corresponding URLO post) that this problem allows one to easily write |
|
wrote a blog post summarizing this issue and adding an example why I believe removing contravariance is not enough to fix this bug. https://lcnr.de/blog/diving-deep-implied-bounds-and-variance/ |
|
I took a brief look into this issue, and it seems rust language contravariance rules and borrow checker can do the needed detection. Here's my inspection, i'm using fn bad<'c, 'd, T>(x: &'c T) -> &'d T {
// instantiate `foo`
let foo1: for<'a, 'b> fn() -> fn(Option<&'a &'b ()>, &'b T) -> &'a T = foo;
// subtyping: instantiate `'b <- 'c`
let foo2: for<'a> fn() -> fn(Option<&'a &'c ()>, &'c T) -> &'a T = foo1;
// subtyping: contravariantly 'c becomes 'static
let foo3: for<'a> fn() -> fn(Option<&'a &'static ()>, &'c T) -> &'a T = foo2;
// subtyping: instantiate `'a <- 'd`
let foo4: fn() -> fn(Option<&'d &'static ()>, &'c T) -> &'d T = foo3;
// boom!
foo4()(None, x)
}So, i implement my fake function pointer type, my own fake use std::marker::PhantomData;
struct Foo<T>(PhantomData<T>);
trait MyFnOnce<Args> {}
impl<'a, 'c, T: 'c> MyFnOnce<(&'a &'c (), &'c T)> for Foo<T> {}
// this compiles
fn good<'c, T: 'c>() {
let foo2: &dyn for<'a> MyFnOnce<(&'a &'c (), &'c T)> = &Foo::<T>(PhantomData);
}
// this doesn't compile
// fn bad<'c, T: 'c>() {
// let foo3: &dyn for<'a> MyFnOnce<(&'a &'static (), &'c T)> = &Foo::<T>(PhantomData);
// }The borrow checker can properly detect the invalid cast within https://github.com/rust-lang/rust/blob/master/compiler/rustc_middle/src/ty/relate.rs#L190-L214 If it can type check similar to my example above, i think the original code will be rejected as expected. EDIT: In case there's extra magic with the builtin #![feature(fn_traits)]
#![feature(unboxed_closures)]
// nightly only
use std::marker::PhantomData;
struct Foo<T>(PhantomData<T>);
impl<'a, 'c, T: 'c> FnOnce<(&'a &'c (), &'c T)> for Foo<T> {
type Output = ();
extern "rust-call"
fn call_once(self, _: (&'a &'c (), &'c T)) -> Self::Output { todo!() }
}
// this compiles
fn bad1<'c, T: 'c>() {
let x: &dyn for<'a> FnOnce(&'a &'c (), &'c T) = &Foo::<T>(PhantomData);
}
// this doesn't
// fn bad2<'c, T: 'c>() {
// let x: &dyn for<'a> FnOnce(&'a &'static (), &'c T) = &Foo::<T>(PhantomData);
// }And for reference, this is the return type example occurred in lcnr's blog. // borrow checker properly handles this too.
// does not compile
// fn my<T: 'static>() {
// let foo1: &dyn for<'a, 'b> FnOnce(&'a (), &'b T) -> (&'a &'b (), &'a T) = todo!();
// let foo2: &dyn for<'b> FnOnce(&'static (), &'b T) -> (&'static &'b (), &'static T) = foo1;
// let foo3: &dyn for<'b> FnOnce(&'static (), &'b T) -> (&'b &'b (), &'static T) = foo2;
//} |
I believe this happens because traits objects are always invariant over their generic arguments. If fn should_compile1<'a, 'f>(f: &'f dyn FnOnce(&'a ())) -> &'f dyn FnOnce(&'static ()) {
f
}
fn should_compile2<'c, F: for<'a, 'b> FnOnce(&'a &'b ())>(f: F) {
let x: &dyn for<'a> FnOnce(&'a &'c ()) = &f;
let y: &dyn for<'a> FnOnce(&'a &'static ()) = x;
} |
|
@SkiFire13 You're correct that, dyn values doesn't suffer this issue because it doesn't use contravariance here. However it seems to me that my trait objects examples still shows that this issue can be solved by removing contravariance, in contradictionary to @lcnr's belief? PS: Personally I'd love to see more aligning between |
|
in my post i have an implementation of // we again have an implied `'input: 'out` bound, this time because of the return type
fn foo<'out, 'input, T>(_dummy: &'out (), value: &'input T) -> (&'out &'input (), &'out T) {
(&&(), value)
}
fn bad<'short, T>(x: &'short T) -> &'static T {
// instantiate `foo`
let foo1: for<'out, 'input> fn(&'out (), &'input T) -> (&'out &'input (), &'out T) = foo;
// instantiate 'out as 'static
let foo2: for<'input> fn(&'static (), &'input T) -> (&'static &'input (), &'static T) = foo1;
// function return types are covariant,
// go from 'static to 'input
let foo3: for<'input> fn(&'static (), &'input T) -> (&'input &'input (), &'static T) = foo2;
// instantiate 'input as 'short
let foo4: fn(&'static (), &'short T) -> (&'short &'short (), &'static T) = foo3;
// boom!
foo4(&(), x).1
}While completely removing variance inside of binders would fix this bug, that seems neither desirable nor feasible, considering the resulting breakage and loss of expressiveness. |
|
@lcnr Would the RFC #27579 https://github.com/rust-lang/rfcs/blob/master/text/1214-projections-lifetimes-and-wf.md fix the issue ? |
|
@GuillaumeDIDIER that RFC already mentions this issue (in this section), saying it will be addressed in a separate RFC. |
add fut/back compat tests for implied trait bounds the `guard` test was tested to cause a segfault with `-Zchalk`, very nice cc `@nikomatsakis` rust-lang#44491 rust-lang#25860
add fut/back compat tests for implied trait bounds the `guard` test was tested to cause a segfault with `-Zchalk`, very nice cc ``@nikomatsakis`` rust-lang#44491 rust-lang#25860
|
To me it looks like function declarations assume all instances of the same lifetime in the signature to be identical. |
show that the buggy implied bounds path is fixed
|
I feel like the issue here isn't fundamental to variance or implied bounds. Let's look at the two more recent examples of this problem. From @arielb1 fn foo<'a, 'b, T>() -> fn(Option<&'a &'b ()>, &'b T) -> &'a T {
fn foo_inner<'a, 'b, T>(_witness: Option<&'a &'b ()>, v: &'b T) -> &'a T {
v
}
foo_inner
}
fn bad<'c, 'd, T>(x: &'c T) -> &'d T {
// instantiate `foo`
let foo1: for<'a, 'b> fn() -> fn(Option<&'a &'b ()>, &'b T) -> &'a T = foo;
// subtyping: instantiate `'b <- 'c`
let foo2: for<'a> fn() -> fn(Option<&'a &'c ()>, &'c T) -> &'a T = foo1;
// subtyping: contravariantly 'c becomes 'static
let foo3: for<'a> fn() -> fn(Option<&'a &'static ()>, &'c T) -> &'a T = foo2;
// subtyping: instantiate `'a <- 'd`
let foo4: fn() -> fn(Option<&'d &'static ()>, &'c T) -> &'d T = foo3;
// boom!
foo4()(None, x)
}And from @lcnr // we again have an implied `'input: 'out` bound, this time because of the return type
fn foo<'out, 'input, T>(_dummy: &'out (), value: &'input T) -> (&'out &'input (), &'out T) {
(&&(), value)
}
fn bad<'short, T>(x: &'short T) -> &'static T {
// instantiate `foo`
let foo1: for<'out, 'input> fn(&'out (), &'input T) -> (&'out &'input (), &'out T) = foo;
// instantiate 'out as 'static
let foo2: for<'input> fn(&'static (), &'input T) -> (&'static &'input (), &'static T) = foo1;
// function return types are covariant,
// go from 'static to 'input
let foo3: for<'input> fn(&'static (), &'input T) -> (&'input &'input (), &'static T) = foo2;
// instantiate 'input as 'short
let foo4: fn(&'static (), &'short T) -> (&'short &'short (), &'static T) = foo3;
// boom!
foo4(&(), x).1
}In both examples, the invalid cast is in the transition from |
Indeed. So implied bounds are clearly fundamental to the problem. |
|
The point I was trying to get across was that it wasn't an insurmountable problem with implied bounds and variance, rather I think it is fixable by incorporating implied bounds into variance in a way. I'm spitballing here because I have no formal experience with this stuff, but is it not possible to add a check along the lines of "T is a subtype of U only if the implied bounds associated with T imply those associated with U." So like with |
Oh, sure. The question is just how to actually implement that. :) |
The combination of variance and implied bounds for nested references opens a hole in the current type system:
This can likely be resolved by checking well-formedness of the instantiated fn type.
Update from @pnkfelix :
While the test as written above is rejected by Rust today (with the error message for line 6 saying "in type
&'static &'a (), reference has a longer lifetime than the data it references"), that is just an artifact of the original source code (with its explicit type signature) running up against one new WF-check.The fundamental issue persists, since one can today write instead:
(and this way, still get the bad behaving
fn bad, by just side-stepping one of the explicit type declarations.)The text was updated successfully, but these errors were encountered: