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