interpret: adjust vtable validity check for higher-ranked types

[lukas-code]

What

Transmuting between trait objects where a generic argument or associated type only differs in bound regions (not bound at or above the trait object's binder) is now UB. For example

• transmuting between &dyn Trait<for<'a> fn(&'a u8)> and &dyn Trait<fn(&'static u8)> is UB.
• transmuting between &dyn Trait<Assoc = for<'a> fn(&'a u8)> and &dyn Trait<Assoc = fn(&'static u8)> is UB.
• transmuting between &dyn Trait<for<'a> fn(&'a u8) -> (&'a u8, &'static u8)> and &dyn Trait<for<'a> fn(&'a u8) -> (&'static u8, &'a u8)> is UB.

Transmuting between subtypes (in either direction) is still allowed, which means that bound regions that are bound at or above the trait object's binder can still be changed:

• transmuting between &dyn for<'a> Trait<fn(&'a u8)> and &dyn for Trait<fn(&'static u8)> is fine.
• transmuting between &dyn for<'a> Trait<dyn Trait<fn(&'a u8)>> and &dyn for Trait<dyn Trait<fn(&'static u8)>> is fine.

Why

Very similar to #120217 and #120222, changing a trait object's generic argument to a type that only differs in bound regions can still affect the vtable layout and lead to segfaults at runtime (for an example see src/tools/miri/tests/fail/validity/dyn-transmute-inner-binder.rs).

Since we already already require that the trait object predicates must be equal modulo bound regions, it is only natural to extend this check to also require type equality considering bound regions.

However, it also makes sense to allow transmutes between a type and a subtype thereof. For example &dyn for<'a> Trait<&'a u8> is a subtype of &dyn Trait<&'static ()> and they are guaranteed to have the same vtable, so it makes sense to allow this transmute. So that's why bound lifetimes that are bound to the trait object itself are treated as free lifetime for the purpose of this check.

Note that codegen already relies on the property that subtyping cannot change the the vtable and this is asserted here (note the leak check):

rust/compiler/rustc_codegen_ssa/src/base.rs

Lines 106 to 153 in 251206c

	/// Codegen takes advantage of the additional assumption, where if the
	/// principal trait def id of what's being casted doesn't change,
	/// then we don't need to adjust the vtable at all. This
	/// corresponds to the fact that `dyn Tr<A>: Unsize<dyn Tr<B>>`
	/// requires that `A = B`; we don't allow *upcasting* objects
	/// between the same trait with different args. If we, for
	/// some reason, were to relax the `Unsize` trait, it could become
	/// unsound, so let's validate here that the trait refs are subtypes.
	pub fn validate_trivial_unsize<'tcx>(
	tcx: TyCtxt<'tcx>,
	source_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>,
	target_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>,
	) -> bool {
	match (source_data.principal(), target_data.principal()) {
	(Some(hr_source_principal), Some(hr_target_principal)) => {
	let (infcx, param_env) =
	tcx.infer_ctxt().build_with_typing_env(ty::TypingEnv::fully_monomorphized());
	let universe = infcx.universe();
	let ocx = ObligationCtxt::new(&infcx);
	infcx.enter_forall(hr_target_principal, |target_principal| {
	let source_principal = infcx.instantiate_binder_with_fresh_vars(
	DUMMY_SP,
	BoundRegionConversionTime::HigherRankedType,
	hr_source_principal,
	);
	let Ok(()) = ocx.eq_trace(
	&ObligationCause::dummy(),
	param_env,
	ToTrace::to_trace(
	&ObligationCause::dummy(),
	hr_target_principal,
	hr_source_principal,
	),
	target_principal,
	source_principal,
	) else {
	return false;
	};
	if !ocx.select_all_or_error().is_empty() {
	return false;
	}
	infcx.leak_check(universe, None).is_ok()
	})
	}
	(_, None) => true,
	_ => false,
	}
	}

Furthermore, we allow some pointer-to-pointer casts like *const dyn for<'a> Trait<&'a u8> to *const Wrapper<dyn Trait<&'static u8>> that instantiate the trait object binder and are currently lowered to a single pointer-to-pointer cast in MIR (CastKind::PtrToPtr) and not an unsizing coercion (CastKind::PointerCoercion(Unsize)), so the current MIR lowering of these would be UB if we didn't allow subtyping transmutes.

---

fixes #135230
cc @rust-lang/opsem
r? @compiler-errors for the implementation

[rustbot]

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

[lcnr]

bivariance is different from being bidirectional subtypes

lcnr/random-rust-snippets#3

change it to eq_modulo_regions? 🤔

👍 on manually adding the leak-check here. The alternative would be to instantiate all regions with existentials and do a proper region check at the end, not totally sure whether a leak-check will always be sufficient given that it doesn't have to be catch all errors

[lukas-code]

bivariance is different from being bidirectional subtypes

Hm, this comment here made me believe that they are supposed to be the same:

rust/compiler/rustc_infer/src/infer/at.rs

Lines 241 to 246 in 336209e

	// We could make this make sense but it's not readily
	// exposed and I don't feel like dealing with it. Note
	// that bivariance in general does a bit more than just
	// *nothing*, it checks that the types are the same
	// "modulo variance" basically.
	ty::Bivariant => panic!("Bivariant given to `relate()`"),

I see now that bivariance as implemented does indeed just do nothing, so 👍 on renaming this to avoid confusion.

---

Also, after thinking about this more, I think we can also just structurally compare the predicates (after instantiating the binder + normalizing), no leak check required, because the soundness of TypeId requires structural equality anyway (#97156).

Is there any reason why this would be a bad idea?

[lcnr]

Hm, this comment here made me believe that they are supposed to be the same:

This comment is wrong then 😁

Also, after thinking about this more, I think we can also just structurally compare the predicates (after instantiating the binder + normalizing), no leak check required, because the soundness of TypeId requires structural equality anyway (#97156).

Is there any reason why this would be a bad idea?

don't think so, would have some false negatives in generic code, but 🤷

[lukas-code]

Alright, replaced this with just == now.

[RalfJung]

Is there a better place for this function? Somewhere in rustc_ty_utils or whatever? I am entirely clueless about its implementation so maybe const_eval is the wrong crate for this. ;)

[lukas-code]

This function is gone now, because turns out that this entire complicated operation is (almost) just equivalent to == ^^

[rustbot]

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

[lukas-code]

(this now includes #135318 to avoid conflicts, sorry for the pings)

[bors]

☔ The latest upstream changes (presumably #135848) made this pull request unmergeable. Please resolve the merge conflicts.

[compiler-errors]

Let's wait for #135318 to land. Sorry for blocking your PR 😓

@rustbot blocked

[lukas-code]

@rustbot review

[compiler-errors]

LGTM. Sorry for taking a bit to review this.

I don't believe this needs an FCP, because it simply fixes a shortcoming in vtable validation code -- this is morally equivalent¹ to changing the eq_in_param_env function to instantiate the binder of the principal trait ref (à la #135318) and then performing a leak check at the end of processing obligations.

The fact that we weren't previously leak-checking (or equivalent) allows certain kinds of transmutes to pass miri today that clearly allow changing the principal trait ref in a way that constitutes different types (e.g. fn(&'static ()) and for<'a> fn(&'a ())).

Footnotes

1. Though simpler b/c we don't need to actually go through the whole dance of using an infcx if we expect the types to be fully normalized and region erased. ↩

[compiler-errors]

cc @rust-lang/opsem and @rust-lang/types once again, if someone does think this needs an FCP then please explain why. Otherwise I'll approve this in a couple of days.

[RalfJung]

Making some more dyn trait type transmutes UB in a very gray area seems entirely reasonable to me. Our reference says that:

dyn Trait metadata must be a pointer to a compiler-generated vtable for Trait. (For raw pointers, this requirement remains a subject of some debate.)

Trait<fn(&'static ())> and Trait<for<'a> fn(&'a ())> are not the same trait. So it is basically a bug that we accepted this so far.

[compiler-errors]

@bors r+

[bors]

📌 Commit a90cb05 has been approved by compiler-errors

It is now in the queue for this repository.