Pointer casts allow switching trait parameters for trait objects, which can be unsound with raw pointers as receiver types under feature(arbitrary_self_types)

[steffahn]

This particular exploitation is possible since #113262. I’m not certain if there wasn’t any way to convince the compiler to do such casts before that; if there wasn’t, then that PR was definitely a lot more than “a bugfix”.

Edit: Turns out, there is a way, see my first answer below. (Does this mean the regression label should be removed, since the regression only extended the scope of the issue a bit? So this becomes an issue for F-arbitrary_self_types in general then? Should we also add requires-nightly label?)

---

So apparently, the compiler doesn’t care about lifetimes for trait object metadata when checking casts between pointers. This means I can coerce *const dyn Foo<'a> into *const dyn Foo<'b> without restrictions. Since vtables logically contain function pointers, like for example a vtable for trait Foo<'a> { fn foo(&self) -> &'a str } logically contains something like unsafe fn(*const ()) -> &'a str this results in casting the types of these function pointers.

Now one could argue “it’s fine, they are raw pointers, you can’t do anything with *const dyn NotQuiteTheRightTrait”, but as far as I’m aware, the story on that is that you can, vtables must be valid (at least as a soundness invariant, not necessarily promising instant UB), and we should not break arbitrary_self_types’s soundness this way, for now.

And with arbitrary_self_types, unsoundness there is!

#![feature(arbitrary_self_types)]

trait Static<'a> {
    fn proof(self: *const Self, s: &'a str) -> &'static str;
}

fn bad_cast<'a>(x: *const dyn Static<'static>) -> *const dyn Static<'a> {
    x as _
}

impl Static<'static> for () {
    fn proof(self: *const Self, s: &'static str) -> &'static str {
        s
    }
}

fn extend_lifetime(s: &str) -> &'static str {
    bad_cast(&()).proof(s)
}

fn main() {
    let s = String::from("Hello World");
    let slice = extend_lifetime(&s);
    println!("Now it exists: {slice}");
    drop(s);
    println!("Now it’s gone: {slice}");
}

Now it exists: Hello World
Now it’s gone: �@7

(playground)

@rustbot label +I-unsound +F-arbitrary_self_types +A-lifetimes +A-coercions +A-trait-objects +requires-nightly

[steffahn]

Uhm… perhaps nevermind on that recent regression point, I guess? Apparently we can also do the following just fine™ since around Rust 1.2

// edition 2015

#![allow(bare_trait_objects)]

trait Trait<T> {}

fn foo(x: *const Trait<u8>) -> *const Trait<u16> {
    x as *const Trait<u16>
}

(Let me work on a better code example for unsoundness from this.)

---

Edit: For example:

#![feature(arbitrary_self_types)]

trait Trait<S, T> {
    fn convert(self: *const Self, x: S) -> T;
}

impl<T> Trait<T, T> for [T; 0] {
    fn convert(self: *const Self, x: T) -> T {
        x
    }
}

fn transmute<S, T>(x: S) -> T {
    (&[] as *const dyn Trait<S, S> as *const dyn Trait<S, T>).convert(x)
}

fn main() {
    let n = 1;
    println!(
        "much data, such joy: {:?}",
        transmute::<&u8, &[u8; u32::MAX as _]>(&n)
    );
}

(playground)

[Noratrieb]

Requiring pointers to have valid vtables while allowing these casts seems pretty conflicting and impossible. cc @rust-lang/opsem @rust-lang/lang who have previously been discussing whether raw pointers should have valid vtables.

[RalfJung]

This wasn't just discussed, it was decided that raw ptrs have valid vtables as a safety invariant as part of dyn trait upcasting.

However, this being about safety invariants, it's more a @rust-lang/types question than an opsem one.

[Noratrieb]

If a safety invariant can be broken by safe code that's uh, not good.

[adetaylor]

Agreed - IMO if arbitrary self types supports calls into *[const|mut] dyn Trait then these should need to be unsafe since it involves dereferencing some data that can be arbitrarily manipulated.

[RalfJung]

Ideally we can fix things so that safe code can't arbitrarily manipulate this.

#120222 is another issue caused by vtable manipulation. Let's see how that one gets resolved (it's a lot more critical since the feature it affects almost got stabilized in 2 weeks), then we can use that to decide what to do about arbitrary-self.

[QuineDot]

This wasn't just discussed, it was decided that raw ptrs have valid vtables as a safety invariant as part of dyn trait upcasting.

Issue #101336 for anyone looking.

[apiraino]

@steffahn can you help me getting a bit more context (maybe I missed some comments and don't have the global picture): how does (if at all) the revert of #118133 (in PR #120233) affect this? Does that "fix" it, at least partially? Thanks :)

[lukas-code]

@apiraino Both feature(arbitrary_self_types) and feature(trait_upcasting) are individually unsound, because we allow casts like *const dyn Trait<u8> -> *const dyn Trait<u16>. But the features are otherwise completely unrelated, so the revert of trait_upcasting does not affect the soundness of arbitrary_self_types at all.

The unsoundness of arbitrary_self_types is tracked in this issue and the unsoundness of trait_upcasting is tracked in #120222.

Both issues could be fixed by disallowing casts like *const dyn Trait<u8> -> *const dyn Trait<u16> (#120222 (comment) / #120248), but that would regress stable code.

An alternative set of fixes is to change the vtable layout to fix trait_upcasting (#120222 (comment)) and to make dispatch on *const dyn Trait unsafe to fix arbitrary_self_types (#120217 (comment)).

[steffahn]

@apiraino The summary from @lukas-code above is already good. As for what the revert of stabilization of trait_upcasting in its current form achieved: all it did was making #120222 from a regression-from-stable-to-beta issue into a requires-nightly issue (which has already been reflected in the labels).

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

[traviscross]

This is believed to be fixed by:

• Make casts of pointers to trait objects stricter #120248