Pointer casts allow switching trait parameters for trait objects, which doesn’t interact soundly with trait upcasting

[steffahn]

Pointer casts allow switching trait parameters for trait objects, which can change the set of supertraits (and thus the vtable layout), ultimately making upcasting of raw pointers quite unsound

This code reproduces a segfault, on upcoming stable Rust, starting in 1.76 (stabilization of trait object upcasting).

Update: Stabilization was reverted, 1.76 stable Rust is good, the below code examples needs nightly Rust and #⁠[feature(trait_upcasting)] now.

#![feature(trait_upcasting)]

pub trait SupSupA {
    fn method(&self) {}
}
pub trait SupSupB {}
impl<T> SupSupA for T {}
impl<T> SupSupB for T {}

pub trait Super<T>: SupSupA + SupSupB {}

pub trait Unimplemented {}

pub trait Trait<T1, T2>: Super<T1> + Super<T2> {
    fn missing_method(&self)
    where
        T1: Unimplemented,
    {
    }
}

impl<S, T> Super<T> for S {}

impl<S, T1, T2> Trait<T1, T2> for S {}

#[inline(never)]
pub fn user1() -> &'static dyn Trait<u8, u8> {
    &()
/* VTABLE:
.L__unnamed_2:
        .quad   core::ptr::drop_in_place<()>
        .asciz  "\000\000\000\000\000\000\000\000\001\000\000\000\000\000\000"
        .quad   example::SupSupA::method
        .quad   .L__unnamed_4 // SupSupB vtable (pointer)
        .zero   8             // null pointer for missing_method
*/
}

#[inline(never)]
pub fn user2() -> &'static dyn Trait<u8, u16> {
    &()
/* VTABLE:
.L__unnamed_3:
        .quad   core::ptr::drop_in_place<()>
        .asciz  "\000\000\000\000\000\000\000\000\001\000\000\000\000\000\000"
        .quad   example::SupSupA::method
        .quad   .L__unnamed_4 // SupSupB vtable (pointer)
        .quad   .L__unnamed_5 // Super<u16> vtable (pointer)
        .zero   8             // null pointer for missing_method
*/
}

fn main() {
    let p: *const dyn Trait<u8, u8> = &();
    let p = p as *const dyn Trait<u8, u16>; // <- this is bad!
    let p = p as *const dyn Super<u16>; // <- this upcast accesses improper vtable entry
    // accessing from L__unnamed_2 the position for the 'Super<u16> vtable (pointer)',
    // thus reading 'null pointer for missing_method'
 
    let p = p as *const dyn SupSupB; // <- this upcast dereferences (null) pointer from that entry
    // to read the SupSupB vtable (pointer)
    
    // SEGFAULT

    println!("{:?}", p);
}

(playground, compiler explorer)

This issue exists next to #120217, but here I’ve found how the issue of overly liberal casting of raw pointers can be made into a concrete soundness issue, using no feature flags, producing an actual segfault.

Whether or now we need (to keep) two separate & somewhat similar GitHub issues can probably become clear eventually. If we don’t, we can always close one or the other.

---

Miri will already complain with a less sophisticated setup:

#![feature(trait_upcasting)]

trait Trait<T>: Super {}
trait Super {}

impl<S, T> Trait<T> for S {}
impl<S> Super for S {}

fn main() {
    let p: *const dyn Trait<u8> = &();
    let p = p as *const dyn Trait<u16>;
    let _p = p as *const dyn Super; // this is where miri complains already
}

(playground)

error: Undefined Behavior: upcast on a pointer whose vtable does not match its type
  --> src/main.rs:12:14
   |
12 |     let _p = p as *const dyn Super; // this is where miri complains already
   |              ^ upcast on a pointer whose vtable does not match its type
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: BACKTRACE:
   = note: inside `main` at src/main.rs:12:14: 12:15

---

Three possible ways of fixing this would be to

• either make vtable layout even more regular,
  • (and to officially allow somewhat “wrong” vtable types, also for miri, and to kill raw pointer[-like] types as arbitrary_self_types receiver…)¹
• or to start working on limiting those pointer casts,
  • (even though they’ve been possible since about Rust 1.2, so some potential breakage and/or transition period, …!?)
• or to disallow upcasting raw pointers, after all.
  • (perhaps also temporarily as a quick fix before pursuing a better approach)

I’m marking as regression in beta as this is newly introduced unsoundness; and an approach like temporarily removing raw pointers again from our upcasting feature (assuming that’s technically possible in the first place) and/or delaying the stabilization are options, in case this soundness issue is perceived as sufficiently relevant not to be “ignored”.

@rustbot label +regression-from-stable-to-beta +F-trait_upcasting +A-coercions +A-trait-objects +I-unsound

Footnotes

1. my thought here is that e.g. it seems like missing methods are replaced with zeroes already, too, instead of skipped, and that pattern could be extended so that in the case of different sets of supertraits (like the Super<u8> == Super<u8> + Super<u8> vs. Super<u8> + Super<u16> above) could thus perhaps also avoid supertrait pointers appearing in inconsistent places between an applied trait constructor [Trait<Args1…> vs Traits<Args2…>] with different parameters ↩

[steffahn]

Between T-lang, T-compiler, and T-types [all 3 listed on the tracking issue], I’m not sure which (or which ones) to assign.

[Noratrieb]

P-critical, I do not want to stabilize a feature involving a new unsoundness

lcnr on Zulip

[RalfJung]

Yes, we should get a de-stabilization PR ASAP to avoid shipping this, and then assess the situation. Is someone working on that?

This seems like @rust-lang/lang @rust-lang/types to me; there's no bug in the compiler that I can see.

[RalfJung]

There's now a revert up at #120233.

[WaffleLapkin]

I think the bug is that we allow casting *const dyn Trait<u8> as *const dyn Trait<u16> (where Trait<u16> is not supertrait of Trait<u8>). Those are different traits, and we generally disallow casting pointers to trait objects with different traits:

trait A {} impl<T> A for T {}
trait B {} impl<T> B for T {}

let a: *const dyn A = &();
let b: *const dyn B = a as _; //~ error: casting `*const dyn A` as `*const dyn B` is invalid

trait Trait<G> {}
struct X; impl<T> Trait<X> for T {}
struct Y; impl<T> Trait<Y> for T {}

let x: *const dyn Trait<X> = &();
let y: *const dyn Trait<Y> = x as _; // ok???

(play)

---

The check is here:

rust/compiler/rustc_hir_typeck/src/cast.rs

Lines 800 to 805 in 366d112

	// vtable kinds must match
	if fcx.tcx.erase_regions(cast_kind) == fcx.tcx.erase_regions(expr_kind) {
	Ok(CastKind::PtrPtrCast)
	} else {
	Err(CastError::DifferingKinds)
	}

Note that we only compare trait def_ids:

rust/compiler/rustc_hir_typeck/src/cast.rs

Line 103 in 366d112

ty::Dynamic(tty, _, ty::Dyn) => Some(PointerKind::VTable(tty.principal_def_id())),

I wander how much breakage will cause the change to this... (upd: I'm working on trying this out)

[lcnr]

• either make vtable layout even more regular,
• (and to officially allow somewhat “wrong” vtable types, also for miri, and to kill raw pointer[-like] types as arbitrary_self_types receiver…)[^1]

I believe this to be fairly straightforward by computing the vtable layout once for a fully generic princial, e.g. reusing the same vtable layout for all instances of for<T, U> dyn Foo<T, U>, regardless of the actual arguments.

I still think we should revert and also try @WaffleLapkin's approach. 2 weeks is not enough time to play with any non-trivial fixes 🤔

[wesleywiser]

Adjusting labels to reflect the reverted stabilization of the feature.

[steffahn]

And probably P-critical prioritization can go away eventually.

[lcnr]

dropping the t-types nomination. the stabilization has been reverted and we now have people working on this. While types team members may still be involved, this does not need any team input rn

[VorpalBlade]

I ran into something where trait upcasting would be the perfect fit. As such I went to look for what was blocking in the tracking PR. It seems that this is the only real blocker? If so, what is the current progress on this (if any)?

[oli-obk]

#120248 is making progress on this

cc @WaffleLapkin

[traviscross]

This is believed to be fixed by:

• Make casts of pointers to trait objects stricter #120248