Fix/issue 152607 supertrait assoc type bound#156593
Conversation
…ormedness When forming a trait object like `dyn Sub<Assoc = String>` where `trait Sub: Super<Assoc: Copy>`, the compiler was not verifying that `String: Copy`. This allowed unsound code to compile, potentially leading to use-after-free vulnerabilities. The root cause was that both the old and new trait solvers used `explicit_super_predicates_of` which only yields `Self: SuperTrait` predicates, but not the associated type bounds from those supertraits. For example, `trait Sub: Super<Assoc: Copy>` expands to both `Self: Super` and `<Self as Super>::Assoc: Copy`, but only the first was being checked. This change switches to `explicit_implied_predicates_of` in both: - The old solver's `confirm_object_candidate` - The new solver's `predicates_for_object_candidate` This ensures that associated type bounds from supertraits are properly verified when constructing trait objects.
|
changes to the core type system cc @lcnr |
This comment has been minimized.
This comment has been minimized.
| for (pred, _) in tcx | ||
| .explicit_implied_predicates_of(principal_def_id) | ||
| .iter_instantiated_copied(tcx, args) | ||
| .map(Unnormalized::skip_norm_wip) | ||
| { | ||
| if !pred.has_escaping_bound_vars() | ||
| && !matches!( | ||
| pred.kind().skip_binder(), | ||
| ty::ClauseKind::Trait(tp) if tp.self_ty() == t | ||
| ) | ||
| { | ||
| self.out.push(traits::Obligation::with_depth( | ||
| tcx, | ||
| self.cause(ObligationCauseCode::WellFormed(None)), | ||
| self.recursion_depth, | ||
| self.param_env, | ||
| pred, | ||
| )); |
There was a problem hiding this comment.
Consider, e.g.:
trait Tr<X>: 'static {}
struct S<'a, X>(&'a dyn Tr<X>);There was a problem hiding this comment.
what exactly do you mean? You refer to the impact of this on implied bounds?
There was a problem hiding this comment.
I have tested that filtering out ty::ClauseKind::TypeOutlives fixes this particular problem. I am not sure if we do not hit something else by this. If this is the correct fix, we should probably also filter ty::ClauseKind::RegionOutlives(..).
There was a problem hiding this comment.
what exactly do you mean?
As @spirali inferred and addressed, this branch had regressed that case.
| .explicit_implied_predicates_of(principal_def_id) | ||
| .iter_instantiated_copied(tcx, args) | ||
| .map(Unnormalized::skip_norm_wip) | ||
| { |
There was a problem hiding this comment.
we should already return these as part of the nominal_obligations and then filter them out, do we not?
I am not 100% sure about this, but would expect so. I don't have a good sense of whether we should filter them out, I feel like it depends on what we want to do with trait objects long term. Their current state is quite a mess sadly
This is not required for soundness, is it? as we do check these bounds when proving dyn Tr: Tr
There was a problem hiding this comment.
I am not sure that I understand. I have originally tried to implement it in the nominal_obligations cycle, but then I do not know how to distinguish the case:
trait Trait<T: Sized> {}
fn foo(_: &dyn Trait<[u32]>) {}AFAIK it should compile as we are just mentioning the type and not constructing it.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Sorry for not getting to this more quickly.. I don't think we have a good sense of what the WF requirements of trait objects should be I don't know about the example by TC, but feel like that example probably should error if we check requirements for trait objects. trait Tr<X>: 'static {}
struct S<X>(*const dyn Tr<X>);
use std::any::Any;
trait Super {
type Assoc;
}
trait Trait: Super<Assoc: 'static> {}
fn foo<T>(x: *mut dyn Trait<Assoc = T>, t: T) -> Box<dyn Any> {
Box::new(t)
}Or well, if you filter out outlives bounds, you could do use std::any::Any;
trait Super {
type Assoc;
}
trait IsStatic: 'static {}
trait Trait: Super<Assoc: IsStatic> {}
fn foo<T>(x: *mut dyn Trait<Assoc = T>, t: T) -> Box<dyn Any> {
Box::new(t)
}if the second one compiles with your change, can you add them as tests? I feel like it's safer for now to not change the well-formedness requirements for trait objects. It is sufficient to check that all their bounds hold when checking |
|
Both examples do not compile. As far as I understand, to allow this case, it would need to implement RFC 2089 to propagate implied bounds. Error message for the first one: Error message for the second one: |
|
I really expected the second one to compile 🤔 ah, the second one fails because we do not imply the trait bound. so while we imply Does the following compile with your change? I think returning use std::any::Any;
trait Super {
type Assoc;
}
trait IsStatic: 'static {}
impl<T: 'static> IsStatic for T {}
trait Trait: Super<Assoc: IsStatic> {}
fn foo<T>(x: *mut dyn Trait<Assoc = T>, t: T) -> Box<dyn Any> {
Box::new(t)
} |
This tries to fix #152607.
This PR is based on #153518.
dyncase, so I have tried to fix it on my own, but I am not sure about the correctness of this fix.r? lcnr