mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Rollup merge of #126090 - compiler-errors:supertrait-assoc-ty-unsoundness, r=lcnr

Browse Source 
Fix supertrait associated type unsoundness

### What?

Object safety allows us to name `Self::Assoc` associated types in certain positions if they come from our trait or one of our supertraits. When this check was implemented, I think it failed to consider that supertraits can have different args, and it was only checking def-id equality.

This is problematic, since we can sneak different implementations in by implementing `Supertrait<NotActuallyTheSupertraitSubsts>` for a `dyn` type. This can be used to implement an unsound transmute function. See the committed test.

### How do we fix it?

We consider the whole trait ref when checking for supertraits. Right now, this is implemented using equality *without* normalization. We erase regions since those don't affect trait selection.

This is a limitation that could theoretically affect code that should be accepted, but doesn't matter in practice -- there are 0 crater regression. We could make this check stronger, but I would be worried about cycle issues. I assume that most people are writing `Self::Assoc` so they don't really care about the trait ref being normalized.

---

### What is up w the stacked commit

This is built on top of https://github.com/rust-lang/rust/pull/122804 though that's really not related, it's just easier to make this modification with the changes to the object safety code that I did in that PR. The only thing is that PR may make this unsoundness slightly easier to abuse, since there are more positions that allow self-associated-types -- I am happy to stall that change until this PR merges.

---

Fixes #126079

r? lcnr
This commit is contained in:
Matthias Krüger 2024-07-26 00:57:20 +02:00 committed by GitHub
commit a88354831b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 322 additions and 116 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

312
compiler/rustc_trait_selection/src/traits/object_safety.rs
View File

@ -12,17 +12,16 @@
use crate::infer::TyCtxtInferExt; use crate::infer::TyCtxtInferExt;
use crate::traits::query::evaluate_obligation::InferCtxtExt; use crate::traits::query::evaluate_obligation::InferCtxtExt;
use crate::traits::{self, Obligation, ObligationCause}; use crate::traits::{util, Obligation, ObligationCause};
use rustc_errors::FatalError; use rustc_errors::FatalError;
use rustc_hir as hir; use rustc_hir as hir;
use rustc_hir::def_id::DefId; use rustc_hir::def_id::DefId;
use rustc_middle::query::Providers; use rustc_middle::query::Providers;
use rustc_middle::ty::{ use rustc_middle::ty::{
self, EarlyBinder, ExistentialPredicateStableCmpExt as _, Ty, TyCtxt, TypeSuperVisitable, self, EarlyBinder, ExistentialPredicateStableCmpExt as _, GenericArgs, Ty, TyCtxt,
TypeVisitable, TypeVisitor, TypeFoldable, TypeFolder, TypeSuperFoldable, TypeSuperVisitable, TypeVisitable,
TypeVisitableExt, TypeVisitor, Upcast,
}; };
use rustc_middle::ty::{GenericArg, GenericArgs};
use rustc_middle::ty::{TypeVisitableExt, Upcast};
use rustc_span::symbol::Symbol; use rustc_span::symbol::Symbol;
use rustc_span::Span; use rustc_span::Span;
use rustc_target::abi::Abi; use rustc_target::abi::Abi;
@ -195,7 +194,13 @@ fn predicates_reference_self(
.predicates .predicates
.iter() .iter()
.map(|&(predicate, sp)| (predicate.instantiate_supertrait(tcx, trait_ref), sp)) .map(|&(predicate, sp)| (predicate.instantiate_supertrait(tcx, trait_ref), sp))
.filter_map(|predicate| predicate_references_self(tcx, predicate)) .filter_map(|(clause, sp)| {
// Super predicates cannot allow self projections, since they're
// impossible to make into existential bounds without eager resolution
// or something.
// e.g. `trait A: B<Item = Self::Assoc>`.
predicate_references_self(tcx, trait_def_id, clause, sp, AllowSelfProjections::No)
})
.collect() .collect()
} }
@ -204,20 +209,25 @@ fn predicates_reference_self(
.in_definition_order() .in_definition_order()
.filter(|item| item.kind == ty::AssocKind::Type) .filter(|item| item.kind == ty::AssocKind::Type)
.flat_map(|item| tcx.explicit_item_bounds(item.def_id).iter_identity_copied()) .flat_map(|item| tcx.explicit_item_bounds(item.def_id).iter_identity_copied())
.filter_map(|c| predicate_references_self(tcx, c)) .filter_map(|(clause, sp)| {
// Item bounds *can* have self projections, since they never get
// their self type erased.
predicate_references_self(tcx, trait_def_id, clause, sp, AllowSelfProjections::Yes)
})
.collect() .collect()
} }
fn predicate_references_self<'tcx>( fn predicate_references_self<'tcx>(
tcx: TyCtxt<'tcx>, tcx: TyCtxt<'tcx>,
(predicate, sp): (ty::Clause<'tcx>, Span), trait_def_id: DefId,
predicate: ty::Clause<'tcx>,
sp: Span,
allow_self_projections: AllowSelfProjections,
) -> Option<Span> { ) -> Option<Span> {
let self_ty = tcx.types.self_param;
let has_self_ty = |arg: &GenericArg<'tcx>| arg.walk().any(|arg| arg == self_ty.into());
match predicate.kind().skip_binder() { match predicate.kind().skip_binder() {
ty::ClauseKind::Trait(ref data) => { ty::ClauseKind::Trait(ref data) => {
// In the case of a trait predicate, we can skip the "self" type. // In the case of a trait predicate, we can skip the "self" type.
data.trait_ref.args[1..].iter().any(has_self_ty).then_some(sp) data.trait_ref.args[1..].iter().any(|&arg| contains_illegal_self_type_reference(tcx, trait_def_id, arg, allow_self_projections)).then_some(sp)
} }
ty::ClauseKind::Projection(ref data) => { ty::ClauseKind::Projection(ref data) => {
// And similarly for projections. This should be redundant with // And similarly for projections. This should be redundant with
@ -235,9 +245,9 @@ fn predicate_references_self<'tcx>(
// //
// This is ALT2 in issue #56288, see that for discussion of the // This is ALT2 in issue #56288, see that for discussion of the
// possible alternatives. // possible alternatives.
data.projection_term.args[1..].iter().any(has_self_ty).then_some(sp) data.projection_term.args[1..].iter().any(|&arg| contains_illegal_self_type_reference(tcx, trait_def_id, arg, allow_self_projections)).then_some(sp)
} }
ty::ClauseKind::ConstArgHasType(_ct, ty) => has_self_ty(&ty.into()).then_some(sp), ty::ClauseKind::ConstArgHasType(_ct, ty) => contains_illegal_self_type_reference(tcx, trait_def_id, ty, allow_self_projections).then_some(sp),
ty::ClauseKind::WellFormed(..) ty::ClauseKind::WellFormed(..)
| ty::ClauseKind::TypeOutlives(..) | ty::ClauseKind::TypeOutlives(..)
@ -383,7 +393,12 @@ fn virtual_call_violations_for_method<'tcx>(
let mut errors = Vec::new(); let mut errors = Vec::new();
for (i, &input_ty) in sig.skip_binder().inputs().iter().enumerate().skip(1) { for (i, &input_ty) in sig.skip_binder().inputs().iter().enumerate().skip(1) {
if contains_illegal_self_type_reference(tcx, trait_def_id, sig.rebind(input_ty)) { if contains_illegal_self_type_reference(
tcx,
trait_def_id,
sig.rebind(input_ty),
AllowSelfProjections::Yes,
) {
let span = if let Some(hir::Node::TraitItem(hir::TraitItem { let span = if let Some(hir::Node::TraitItem(hir::TraitItem {
kind: hir::TraitItemKind::Fn(sig, _), kind: hir::TraitItemKind::Fn(sig, _),
.. ..
@ -396,7 +411,12 @@ fn virtual_call_violations_for_method<'tcx>(
errors.push(MethodViolationCode::ReferencesSelfInput(span)); errors.push(MethodViolationCode::ReferencesSelfInput(span));
} }
} }
if contains_illegal_self_type_reference(tcx, trait_def_id, sig.output()) { if contains_illegal_self_type_reference(
tcx,
trait_def_id,
sig.output(),
AllowSelfProjections::Yes,
) {
errors.push(MethodViolationCode::ReferencesSelfOutput); errors.push(MethodViolationCode::ReferencesSelfOutput);
} }
if let Some(code) = contains_illegal_impl_trait_in_trait(tcx, method.def_id, sig.output()) { if let Some(code) = contains_illegal_impl_trait_in_trait(tcx, method.def_id, sig.output()) {
@ -482,7 +502,7 @@ fn virtual_call_violations_for_method<'tcx>(
return false; return false;
} }
contains_illegal_self_type_reference(tcx, trait_def_id, pred) contains_illegal_self_type_reference(tcx, trait_def_id, pred, AllowSelfProjections::Yes)
}) { }) {
errors.push(MethodViolationCode::WhereClauseReferencesSelf); errors.push(MethodViolationCode::WhereClauseReferencesSelf);
} }
@ -711,121 +731,181 @@ fn receiver_is_dispatchable<'tcx>(
infcx.predicate_must_hold_modulo_regions(&obligation) infcx.predicate_must_hold_modulo_regions(&obligation)
} }
#[derive(Copy, Clone)]
enum AllowSelfProjections {
Yes,
No,
}
/// This is somewhat subtle. In general, we want to forbid
/// references to `Self` in the argument and return types,
/// since the value of `Self` is erased. However, there is one
/// exception: it is ok to reference `Self` in order to access
/// an associated type of the current trait, since we retain
/// the value of those associated types in the object type
/// itself.
///
/// ```rust,ignore (example)
/// trait SuperTrait {
/// type X;
/// }
///
/// trait Trait : SuperTrait {
/// type Y;
/// fn foo(&self, x: Self) // bad
/// fn foo(&self) -> Self // bad
/// fn foo(&self) -> Option<Self> // bad
/// fn foo(&self) -> Self::Y // OK, desugars to next example
/// fn foo(&self) -> <Self as Trait>::Y // OK
/// fn foo(&self) -> Self::X // OK, desugars to next example
/// fn foo(&self) -> <Self as SuperTrait>::X // OK
/// }
/// ```
///
/// However, it is not as simple as allowing `Self` in a projected
/// type, because there are illegal ways to use `Self` as well:
///
/// ```rust,ignore (example)
/// trait Trait : SuperTrait {
/// ...
/// fn foo(&self) -> <Self as SomeOtherTrait>::X;
/// }
/// ```
///
/// Here we will not have the type of `X` recorded in the
/// object type, and we cannot resolve `Self as SomeOtherTrait`
/// without knowing what `Self` is.
fn contains_illegal_self_type_reference<'tcx, T: TypeVisitable<TyCtxt<'tcx>>>( fn contains_illegal_self_type_reference<'tcx, T: TypeVisitable<TyCtxt<'tcx>>>(
tcx: TyCtxt<'tcx>, tcx: TyCtxt<'tcx>,
trait_def_id: DefId, trait_def_id: DefId,
value: T, value: T,
allow_self_projections: AllowSelfProjections,
) -> bool { ) -> bool {
// This is somewhat subtle. In general, we want to forbid value
// references to `Self` in the argument and return types, .visit_with(&mut IllegalSelfTypeVisitor {
// since the value of `Self` is erased. However, there is one tcx,
// exception: it is ok to reference `Self` in order to access trait_def_id,
// an associated type of the current trait, since we retain supertraits: None,
// the value of those associated types in the object type allow_self_projections,
// itself. })
// .is_break()
// ```rust }
// trait SuperTrait {
// type X;
// }
//
// trait Trait : SuperTrait {
// type Y;
// fn foo(&self, x: Self) // bad
// fn foo(&self) -> Self // bad
// fn foo(&self) -> Option<Self> // bad
// fn foo(&self) -> Self::Y // OK, desugars to next example
// fn foo(&self) -> <Self as Trait>::Y // OK
// fn foo(&self) -> Self::X // OK, desugars to next example
// fn foo(&self) -> <Self as SuperTrait>::X // OK
// }
// ```
//
// However, it is not as simple as allowing `Self` in a projected
// type, because there are illegal ways to use `Self` as well:
//
// ```rust
// trait Trait : SuperTrait {
// ...
// fn foo(&self) -> <Self as SomeOtherTrait>::X;
// }
// ```
//
// Here we will not have the type of `X` recorded in the
// object type, and we cannot resolve `Self as SomeOtherTrait`
// without knowing what `Self` is.
struct IllegalSelfTypeVisitor<'tcx> { struct IllegalSelfTypeVisitor<'tcx> {
tcx: TyCtxt<'tcx>, tcx: TyCtxt<'tcx>,
trait_def_id: DefId, trait_def_id: DefId,
supertraits: Option<Vec<DefId>>, supertraits: Option<Vec<ty::TraitRef<'tcx>>>,
} allow_self_projections: AllowSelfProjections,
}
impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for IllegalSelfTypeVisitor<'tcx> { impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for IllegalSelfTypeVisitor<'tcx> {
type Result = ControlFlow<()>; type Result = ControlFlow<()>;
fn visit_ty(&mut self, t: Ty<'tcx>) -> Self::Result { fn visit_ty(&mut self, t: Ty<'tcx>) -> Self::Result {
match t.kind() { match t.kind() {
ty::Param(_) => { ty::Param(_) => {
if t == self.tcx.types.self_param { if t == self.tcx.types.self_param {
ControlFlow::Break(()) ControlFlow::Break(())
} else { } else {
ControlFlow::Continue(())
}
}
ty::Alias(ty::Projection, ref data)
if self.tcx.is_impl_trait_in_trait(data.def_id) =>
{
// We'll deny these later in their own pass
ControlFlow::Continue(()) ControlFlow::Continue(())
} }
ty::Alias(ty::Projection, ref data) => {
// This is a projected type `<Foo as SomeTrait>::X`.
// Compute supertraits of current trait lazily.
if self.supertraits.is_none() {
let trait_ref =
ty::Binder::dummy(ty::TraitRef::identity(self.tcx, self.trait_def_id));
self.supertraits = Some(
traits::supertraits(self.tcx, trait_ref).map(|t| t.def_id()).collect(),
);
}
// Determine whether the trait reference `Foo as
// SomeTrait` is in fact a supertrait of the
// current trait. In that case, this type is
// legal, because the type `X` will be specified
// in the object type. Note that we can just use
// direct equality here because all of these types
// are part of the formal parameter listing, and
// hence there should be no inference variables.
let is_supertrait_of_current_trait = self
.supertraits
.as_ref()
.unwrap()
.contains(&data.trait_ref(self.tcx).def_id);
// only walk contained types if it's not a super trait
if is_supertrait_of_current_trait {
ControlFlow::Continue(())
} else {
t.super_visit_with(self) // POSSIBLY reporting an error
}
}
_ => t.super_visit_with(self), // walk contained types, if any
} }
} ty::Alias(ty::Projection, ref data) if self.tcx.is_impl_trait_in_trait(data.def_id) => {
// We'll deny these later in their own pass
ControlFlow::Continue(())
}
ty::Alias(ty::Projection, ref data) => {
match self.allow_self_projections {
AllowSelfProjections::Yes => {
// This is a projected type `<Foo as SomeTrait>::X`.
fn visit_const(&mut self, ct: ty::Const<'tcx>) -> Self::Result { // Compute supertraits of current trait lazily.
// Constants can only influence object safety if they are generic and reference `Self`. if self.supertraits.is_none() {
// This is only possible for unevaluated constants, so we walk these here. self.supertraits = Some(
self.tcx.expand_abstract_consts(ct).super_visit_with(self) util::supertraits(
self.tcx,
ty::Binder::dummy(ty::TraitRef::identity(
self.tcx,
self.trait_def_id,
)),
)
.map(|trait_ref| {
self.tcx.erase_regions(
self.tcx.instantiate_bound_regions_with_erased(trait_ref),
)
})
.collect(),
);
}
// Determine whether the trait reference `Foo as
// SomeTrait` is in fact a supertrait of the
// current trait. In that case, this type is
// legal, because the type `X` will be specified
// in the object type. Note that we can just use
// direct equality here because all of these types
// are part of the formal parameter listing, and
// hence there should be no inference variables.
let is_supertrait_of_current_trait =
self.supertraits.as_ref().unwrap().contains(
&data.trait_ref(self.tcx).fold_with(
&mut EraseEscapingBoundRegions {
tcx: self.tcx,
binder: ty::INNERMOST,
},
),
);
// only walk contained types if it's not a super trait
if is_supertrait_of_current_trait {
ControlFlow::Continue(())
} else {
t.super_visit_with(self) // POSSIBLY reporting an error
}
}
AllowSelfProjections::No => t.super_visit_with(self),
}
}
_ => t.super_visit_with(self),
} }
} }
value fn visit_const(&mut self, ct: ty::Const<'tcx>) -> Self::Result {
.visit_with(&mut IllegalSelfTypeVisitor { tcx, trait_def_id, supertraits: None }) // Constants can only influence object safety if they are generic and reference `Self`.
.is_break() // This is only possible for unevaluated constants, so we walk these here.
self.tcx.expand_abstract_consts(ct).super_visit_with(self)
}
}
struct EraseEscapingBoundRegions<'tcx> {
tcx: TyCtxt<'tcx>,
binder: ty::DebruijnIndex,
}
impl<'tcx> TypeFolder<TyCtxt<'tcx>> for EraseEscapingBoundRegions<'tcx> {
fn cx(&self) -> TyCtxt<'tcx> {
self.tcx
}
fn fold_binder<T>(&mut self, t: ty::Binder<'tcx, T>) -> ty::Binder<'tcx, T>
where
T: TypeFoldable<TyCtxt<'tcx>>,
{
self.binder.shift_in(1);
let result = t.super_fold_with(self);
self.binder.shift_out(1);
result
}
fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> {
if let ty::ReBound(debruijn, _) = *r
&& debruijn < self.binder
{
r
} else {
self.tcx.lifetimes.re_erased
}
}
} }
pub fn contains_illegal_impl_trait_in_trait<'tcx>( pub fn contains_illegal_impl_trait_in_trait<'tcx>(

60
tests/ui/object-safety/almost-supertrait-associated-type.rs Normal file
View File

@ -0,0 +1,60 @@
// Test for fixed unsoundness in #126079.
// Enforces that the associated types that are object safe
use std::marker::PhantomData;
fn transmute<T, U>(t: T) -> U {
(&PhantomData::<T> as &dyn Foo<T, U>).transmute(t)
//~^ ERROR the trait `Foo` cannot be made into an object
//~| ERROR the trait `Foo` cannot be made into an object
}
struct ActuallySuper;
struct NotActuallySuper;
trait Super<Q> {
type Assoc;
}
trait Dyn {
type Out;
}
impl<T, U> Dyn for dyn Foo<T, U> + '_ {
//~^ ERROR the trait `Foo` cannot be made into an object
type Out = U;
}
impl<S: Dyn<Out = U> + ?Sized, U> Super<NotActuallySuper> for S {
type Assoc = U;
}
trait Foo<T, U>: Super<ActuallySuper, Assoc = T>
where
<Self as Mirror>::Assoc: Super<NotActuallySuper>
{
fn transmute(&self, t: T) -> <Self as Super<NotActuallySuper>>::Assoc;
}
trait Mirror {
type Assoc: ?Sized;
}
impl<T: ?Sized> Mirror for T {
type Assoc = T;
}
impl<T, U> Foo<T, U> for PhantomData<T> {
fn transmute(&self, t: T) -> T {
t
}
}
impl<T> Super<ActuallySuper> for PhantomData<T> {
type Assoc = T;
}
impl<T> Super<NotActuallySuper> for PhantomData<T> {
type Assoc = T;
}
fn main() {
let x = String::from("hello, world");
let s = transmute::<&str, &'static str>(x.as_str());
drop(x);
println!("> {s}");
}

55
tests/ui/object-safety/almost-supertrait-associated-type.stderr Normal file
View File

@ -0,0 +1,55 @@
error[E0038]: the trait `Foo` cannot be made into an object
--> $DIR/almost-supertrait-associated-type.rs:21:20
|
LL | impl<T, U> Dyn for dyn Foo<T, U> + '_ {
| ^^^^^^^^^^^^^^^^^^ `Foo` cannot be made into an object
|
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
--> $DIR/almost-supertrait-associated-type.rs:33:34
|
LL | trait Foo<T, U>: Super<ActuallySuper, Assoc = T>
| --- this trait cannot be made into an object...
...
LL | fn transmute(&self, t: T) -> <Self as Super<NotActuallySuper>>::Assoc;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ...because method `transmute` references the `Self` type in its return type
= help: consider moving `transmute` to another trait
= help: only type `std::marker::PhantomData<T>` implements the trait, consider using it directly instead
error[E0038]: the trait `Foo` cannot be made into an object
--> $DIR/almost-supertrait-associated-type.rs:7:27
|
LL | (&PhantomData::<T> as &dyn Foo<T, U>).transmute(t)
| ^^^^^^^^^^^^^^ `Foo` cannot be made into an object
|
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
--> $DIR/almost-supertrait-associated-type.rs:33:34
|
LL | trait Foo<T, U>: Super<ActuallySuper, Assoc = T>
| --- this trait cannot be made into an object...
...
LL | fn transmute(&self, t: T) -> <Self as Super<NotActuallySuper>>::Assoc;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ...because method `transmute` references the `Self` type in its return type
= help: consider moving `transmute` to another trait
= help: only type `std::marker::PhantomData<T>` implements the trait, consider using it directly instead
error[E0038]: the trait `Foo` cannot be made into an object
--> $DIR/almost-supertrait-associated-type.rs:7:6
|
LL | (&PhantomData::<T> as &dyn Foo<T, U>).transmute(t)
| ^^^^^^^^^^^^^^^^^ `Foo` cannot be made into an object
|
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
--> $DIR/almost-supertrait-associated-type.rs:33:34
|
LL | trait Foo<T, U>: Super<ActuallySuper, Assoc = T>
| --- this trait cannot be made into an object...
...
LL | fn transmute(&self, t: T) -> <Self as Super<NotActuallySuper>>::Assoc;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ...because method `transmute` references the `Self` type in its return type
= help: consider moving `transmute` to another trait
= help: only type `std::marker::PhantomData<T>` implements the trait, consider using it directly instead
= note: required for the cast from `&PhantomData<T>` to `&dyn Foo<T, U>`
error: aborting due to 3 previous errors
For more information about this error, try `rustc --explain E0038`.

11
tests/ui/object-safety/item-bounds-can-reference-self.rs Normal file
View File

@ -0,0 +1,11 @@
//@ check-pass
pub trait Foo {
type X: PartialEq;
type Y: PartialEq<Self::Y>;
type Z: PartialEq<Self::Y>;
}
fn uwu(x: &dyn Foo<X = i32, Y = i32, Z = i32>) {}
fn main() {}