Inconsistent/wrong choice of trait impls under miri with transmuted vtables
#135230
Labels
A-miri
Area: The miri tool
A-trait-objects
Area: trait objects, vtable layout
C-bug
Category: This is a bug.
I-ICE
Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
rustbot
added
needs-triage
|
Having not checked any of the compiler code yet, I suspect the inconsistency here is in the way const-eval lazily calls |
saethlin
removed
the
needs-triage
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this issue
Feb 20, 2025
Rollup merge of rust-lang#135296 - lukas-code:dyn-leak-check, r=compiler-errors interpret: adjust vtable validity check for higher-ranked types ## 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 rust-lang#120217 and rust-lang#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): https://github.com/rust-lang/rust/blob/251206c27b619ccf3a08e2ac4c525dc343f08492/compiler/rustc_codegen_ssa/src/base.rs#L106-L153 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 rust-lang#135230 cc `@rust-lang/opsem` r? `@compiler-errors` for the implementation
RalfJung
pushed a commit
to RalfJung/miri
that referenced
this issue
Feb 24, 2025
interpret: adjust vtable validity check for higher-ranked types ## 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 rust-lang/rust#120217 and rust-lang/rust#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): https://github.com/rust-lang/rust/blob/251206c27b619ccf3a08e2ac4c525dc343f08492/compiler/rustc_codegen_ssa/src/base.rs#L106-L153 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 rust-lang/rust#135230 cc `@rust-lang/opsem` r? `@compiler-errors` for the implementation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Here is a test case
(playground)
When run normally, this prints
When run with miri, this prints
Expected behavior: Miri either reports UB, or behaves the same way like the actual codegen does.
It gets even more interesting if we add a trait bound to the
impl:(playground)
When run normally, this prints
When run with miri, this prints
Oh wonderful, the type apparently just changes in the middle of it!
If we add a second implementation, Miri reconsiders its choices
(playground)
(Behavior when run normally: unchanged.)
When run with miri, this prints
(still wrong though / [actual execution without
miriis reportingMarker1,Marker1])This behavior must come from some sort of rough heuristic that wasn't supposed to ever matter… because… if the second
implexists but comes with an impossible trait bound, then miri still seems to try to make use of thisMarker2-impl nonetheless:(playground)
When run normally, this still prints
When run with miri, you get ICE:
To run into this ICE, calling a method of
Boundisn't actually necessary. Once they.report()call it reached, it already goes ICE:(playground)
@rustbot label A-miri, A-trait-objects, I-ICE, T-compiler
The text was updated successfully, but these errors were encountered: