arbitrary_self_types + derive_coerce_pointee allows calling methods whose where clauses are violated

[RalfJung]

Here's an example by @steffahn:

#![forbid(unsafe_code)]
#![feature(arbitrary_self_types, derive_coerce_pointee)]

use std::any::TypeId;
use std::marker::CoercePointee;
use std::marker::PhantomData;

#[derive(CoercePointee)]
#[repr(transparent)]
struct SelfPtr<T: ?Sized>(*const T);

impl<T: ?Sized> std::ops::Deref for SelfPtr<T> {
    type Target = T;
    fn deref(&self) -> &T {
        panic!("please don't call me, I just want the `Receiver` impl!");
    }
}

trait GetTypeId {
    fn get_type_id(self: SelfPtr<Self>) -> TypeId
    where
        Self: 'static;
}

impl<T: ?Sized> GetTypeId for PhantomData<T> {
    fn get_type_id(self: SelfPtr<Self>) -> TypeId
    where
        Self: 'static,
    {
        TypeId::of::<T>()
    }
}

// no `T: 'static` bound 🐈‍⬛ necessary
fn type_id_of<T: ?Sized>() -> TypeId {
    SelfPtr(&PhantomData::<T> as *const (dyn GetTypeId + '_) as *const (dyn GetTypeId + 'static)).get_type_id()
}
fn type_id_of_val<T: ?Sized>(_: &T) -> TypeId {
    type_id_of::<T>()
}

fn main() {
    let mut x = 1234;
    let reference = &mut x;
    let tid_of_ref = type_id_of_val(&reference);
    let closure = || *reference += 1;
    let tid_of_closure = type_id_of_val(&closure);
    dbg!(tid_of_ref, tid_of_closure);
}

The double as *const dyn GetTypeId as *const dyn GetTypeId is necessary; the second as changes the lifetime something short-lived to 'static. That means we call get_type_id on a type that does not satisfy Self: 'static.

@BoxyUwU has a similar example:

#![feature(arbitrary_self_types, derive_coerce_pointee)]

use std::marker::CoercePointee;

#[derive(CoercePointee)]
#[repr(transparent)]
struct MyPtr<T: ?Sized>(*const T);

use std::ops::Receiver;

impl<T: ?Sized> Receiver for MyPtr<T> {
    type Target = T;
}

trait Trait {
    fn foo(self: MyPtr<Self>)
    where
        Self: Send;
}

impl Trait for *mut () {
    fn foo(self: MyPtr<Self>) {
        unreachable!()
    }
}

fn main() {
    let a = 0x1 as *const *mut ();
    let a = a as *const dyn Trait as *const (dyn Trait + Send);
    MyPtr(a).foo();
}

Boxy's example is mitigated by a FCW lint. However, the lifetime example doesn't trigger a lint, and while the code above is "fine", we should at least have a good idea for why this cannot cause UB in other ways. (My gut feeling is that this can cause UB but I haven't tried.)

Cc @rust-lang/types

[traviscross]

cc @adetaylor @rust-lang/lang

[steffahn]

My gut feeling is that this can cause UB but I haven't tried.

I've had the same feeling about this and then I did try for a while and didn't find a way to make this UB. Which proves nothing, but changed my gut feeling to “this might be fine”.

[compiler-errors]

I think this boils down essentially to the question: Can you use an impossible non-higher-ranked type-outlives where clause to do UB if you don't have a value of the type?

That's because for you to do something sneaky, for example, you'd need to have a value of the type to do something with that type-outlives where clause (e.g. move it into a thread), and you can't do that without that type showing up in a function signature, and you can't do that (in an dyn-compatible way) unless it shows up in the args of a dyn Trait<...>, at which point the 'static where clause doesn't hold bc dyn Tr<A, B, ...> + 'o: 'static only if 'o: 'static and all A: 'static, B: 'static, ....

So I'm semi-reasonably convinced this is not possible yet. However, it does still fill me with a bit of worry that we don't change the way implied bounds work or something that could make this UB in the future...

---

One way we could prevent this becoming a problem is by strengthening the derive(CoercePointee)/DispatchFromDyn impl validation to not allow dispatching to *const T/*mut T inner pointer types without the arbitrary_self_types_pointers feature gate. This essentially punts the question onwards to that feature gate.

[Darksonn]

One way we could prevent this becoming a problem is by strengthening the derive(CoercePointee)/DispatchFromDyn impl validation to not allow dispatching to *const T/*mut T inner pointer types without the arbitrary_self_types_pointers feature gate. This essentially punts the question onwards to that feature gate.

The primary motivating use-case for derive(CoercePointee) is Arc, which is implemented as a wrapper around a raw pointer.

[RalfJung]

The other option would be to disallow changing that lifetime in as casts, just like we already disallow changing marker traits. If raw pointers have an important invariant about their vtable, we shouldn't allow safe casts to alter things which are relevant for that vtable.

[Darksonn]

My guess is that disallowing lifetime casts in general would be too breaking, but perhaps we could do it only for traits with a function that has a trait bound that implies Self: 'static.

[Darksonn]

The other option here is to make #[derive(CoercePointee)] unsafe, but fundamentally I think that permitting the lifetime cast goes against what was decided in #101336. We shouldn't permit you to make casts that change which vtable functions are available.

[Darksonn]

I just had an idea: What about we disallow all raw pointer casts of raw dyn pointers (both lifetime and auto trait case), but only when the trait has any methods with a where clause on it?

[compiler-errors]

That seems pretty arbitrary, and also interacts poorly with adding certain kinds of trivial where clauses to methods which today which is not a breaking change. I'd really rather we don't try to design some fix for this that's cookie-cutter designed just for this problem.

[BoxyUwU]

For the record we already forbid casting dyn Trait<'a> to dyn Trait<'b>. So the only thing here is casting of the trait object lifetime. I would like to see how breaking forbidding object lifetime casts is in practice and see if we can make it a FCW which I think would be fine for stabilization if we think this can't be exploited in practice

[steffahn]

@BoxyUwU is this even possible as a FCW? Realistically, it’ll create borrow-check errors, I’d be surprised if the borrow checker supports conditions like “I want 'a: 'b, but not really, but pls warn if this constraint isn’t met”

[Darksonn]

I've seen the cast been used a fair number of times with dyn Future to pass it across thread boundaries in cases where the lifetime is enforced in other ways. For example, this is common when people implement their own scoped spawns.

I guess the real rule here is that if you cast from dyn A to dyn B where dyn B has callable methods that dyn A does not, then that's a problem. Even if the concrete type does have the new method that's only on dyn B, we could imagine that the vtable doesn't have it since the vtable was generated for dyn A.

So the precise check we need is to go through each method on the trait and verify that if it's callable on dyn B, then it's also callable on dyn A.

[adetaylor]

@Darksonn are you proposing that the check you describe is done at the time of the cast, or at method call time?

I've been digging into what a "valid vtable pointer" might mean, and this example surprises me:

use std::any::TypeId;
use std::marker::PhantomData;

#[allow(dead_code)]
trait GetTypeId {
    fn foo(&self);
    fn get_type_id(&self) -> TypeId
    where
        Self: 'static;
}

impl<T: ?Sized> GetTypeId for PhantomData<T> {
    fn foo(&self) {}
    fn get_type_id(&self) -> TypeId
    where
        Self: 'static,
    {
        TypeId::of::<T>()
    }
}

fn make_bad_get_type_id<T: ?Sized>(_: &T) -> *const (dyn GetTypeId + 'static) {
    &PhantomData::<T> as *const (dyn GetTypeId + '_) as *const (dyn GetTypeId + 'static)
}

fn make_good_get_type_id<'a, T: ?Sized>(_: &'a T) -> *const (dyn GetTypeId + 'a) {
    &PhantomData::<T> as *const dyn GetTypeId
}

fn main() {
    let mut x = 1234;
    let reference = &mut x;
    let ok_tid_of_ref = make_good_get_type_id(reference);
    let rf = unsafe { &*ok_tid_of_ref };
    // let ok_type_id = rf.get_type_id(); // does not compile
    let bad_tid_of_ref = make_bad_get_type_id(reference);
    let rf = unsafe { &*bad_tid_of_ref };
    let bad_type_id = rf.get_type_id();
    dbg!(bad_type_id);
}

The surprises are:

• The compiler generates identical vtables for ok_tid_of_ref and bad_tid_of_ref, even though there's no legal way to call get_type_id on bad_tid_of_ref.
• The line marked "does not compile" doesn't compile. Somehow, the lifetime information persists through the raw pointer so that the compiler knows it can't allow calls to get_type_id.

I'm assuming your intention is that the checks you propose are done at cast-time, whereas to be consistent with the above behavior, maybe it's OK to allow *const dyn SomethingInvalid to exist, but then we need to somehow check the lifetime information within it at method-call time. I'm not sure how to do that. Advice welcome.

[Darksonn]

My intent is that the checks I propose are done at cast time. This reinforces that even raw pointers must always have a valid vtable for the type when they are wide. This is consistent with #101336.

As for your examples, *const (dyn GetTypeId + 'static) has a vtable with two valid functions in it, whereas *const (dyn GetTypeId + 'a) has a vtable containing one valid function, and an invalid function. Casting the latter to *const (dyn GetTypeId + 'static) involves treating an invalid function to a valid one, which is not okay if we require raw pointers to hold a valid vtable.

[compiler-errors]

Please be aware that type- and lifetime-outlives where clauses may never be used to disqualify a trait from a vtable, since we erase lifetime information post-borrowck we literally do not (and will never) have the information to know whether it may hold or not. So no, today *const (dyn GetTypeId + 'a) has two valid functions in its vtable regardless of the value of 'a.

[Darksonn]

The entire crux of this issue is that calling the get_type_id function pointer in the vtable of *const (dyn GetTypeId + 'a) is not okay and never will be, because the where clauses are not satisfied. Surely that implies that the get_type_id function in the vtable should be treated as invalid? That's the case, even if we happen to put a valid function in that vtable entry due to implementation details related to lifetime erasure.

Or do you mean to say that it's okay to call the get_type_id function pointer in the vtable of *const (dyn GetTypeId + 'a)?

[compiler-errors]

The entire crux of this issue is that calling the get_type_id function pointer in the vtable of *const (dyn GetTypeId + 'a) is not okay and never will be, because the where clauses are not satisfied

Yes, for some substitutions of 'a it's not valid to call get_type_id, but obviously not all substitutions of 'a (e.g. 'a = 'static). That means that the only place I can truly verify whether a method is going to be impossible to call ever is post-monomorphization. There is a distinction here, though, because lifetimes are erased after borrowck. Unlike constructing vtables with impossible trait where clauses where we have to re-check those where clauses post-mono, this is not possible to do for type- and lifetime-outlives since we simply do not have sufficient information during codegen time.

The notion that lifetime generics in general are not treated on the same footing as type and const generics in the type system is pervasive, and type- and lifetime-outlives where clauses are never practically going to be checkable in codegen.

All that we can do with lifetimes is gather their side-effects during borrowck and validate them; this is why what Boxy's checking in #136776 is likely going to be the only way forward here.

[nikomatsakis]

I'm trying to understand exactly what this example is showing me and in what way it truly requires arbitrary-self-types.

I guess the wacky thing we are now able to do is to combine

(1) the pre-existing rules that allow you to cast a *X to *Y without much of a safety check;
(2) with the fact that you can a *const dyn;
(3) with the fact that you can then dispatch based on self-type (potentially through dyn, even) without unsafe?

[Darksonn]

Basically by combining the two features, you can call a function pointer in the vtable of a raw pointer safely.

[nikomatsakis]

Yeah. I regret permitting the cast between raw pointers in quite the way we do, but I feel like this is the "heart of the problem"

[steffahn]

If we focus on the case of parameter-less trait bounds (which also looks like it’s the most common use-case) [and perhaps additionally traits with type parameters that are : 'static; or more concretely even lifetime-free, so e.g. the () parameter of FnOnce() is covered] then the question of soundness boils down to the question of:

• will it always be true that every type T has a corresponding type T0: 'static, identical in representation, obtained by “replacing all free lifetime in T with 'static”
  • in particular, could this “replace all free lifetimes with'static” result in an invalid type (e.g. violation of implied bounds) – or more generally, considering other trait bounds on the methods are possible, could any trait bound (or constraint) that doesn't involve other free lifetimes, be met by T but not by T0?
• even if all of the above is true, would exposing this fact perhaps still be able to break certain API’s soundness, anyway?
  • probably very technically that’s a clear yes; e.g. an API could offer a sealed trait Foo and no visible implementor; and some function fn foo<T: Foo + 'static> that’s UB to call; and another function fn bar(f: impl Callback) calling some trait Callback { fn call<T: Foo>(); }.
    This way the user only ever sees an implementor of Foo through this generic API, and this generic API never provides T: 'static.
    The assumption is thus that foo is sound because it’s impossible to call (using safe Rust), but with a way to turn T: Foo into a T0: 'static + Foo, it becomes callable.¹
    • the relevant question is if any practical API might want to make reasonably want to depend on these soundness assumptions?

If all of the above is true / unproblematic, one could even expose this via a trait, I suppose

trait RemoveFreeLifetimes {
    type WithoutFreeLifetimes: ?Sized + 'static;
}
// automatically implemented by the compiler
// for every type `T: ?Sized`

Then for any type T, the abovementioned corresponding T0 is <T as RemoveFreeLifetimes>::WithoutFreeLifetimes.

(With such an API, you still don’t really express the “T: TraitBound”-implies-T0: TraitBound relation for lifetime-free TraitBounds)

One can furthermore think of *const T as *const dyn Trait as *const (dyn Trait + 'static) as equivalent to *const T as *const <T as RemoveFreeLifetimes>::WithoutFreeLifetimes as *const (dyn Trait + 'static).

---

I’m not sure if the future-compatibility concerns @compiler-errors mentioned would also apply to this case of parameter-less (or lifetime-free-only parameter) trait bounds. Though it’s also not clear to me whether it’s technically possible to begin with² to introduce an error only for cases outside of this more limited scope.

Footnotes

1. This should be possible to do by using *const T to *const dyn Trait to *const dyn (Trait + 'static) casts with some trait Trait { fn call(self: *const Self) where Self: 'static; } and a impl<T: Foo> Trait for Foo { fn call(self: *const Self) where Self: 'static { foo::<Self>(); } }, relying on the arbitrary-self-type + casts mechanism this issue is about. ↩

2. While also not changing the semver contracts of Rust ↩

[steffahn]

which must have been several weeks ago

nevermind that… months ago: https://rust-lang.zulipchat.com/#narrow/channel/136281-t-opsem/topic/Can.20.60*mut.20dyn.20T.20.2B.20'a.20-.3E.20*mut.20dyn.20T.20.2B.20'b.60.20cast.20be.20exploited.3F/near/424767839

Edit: Actually, I think both might be true, I do believe that when I came up with the TypeId example stuff last month, I re-did some of this exploration. Yay for re-doing work of one’s past self 😅

[traviscross]

Above, in his second bullet point in #136702 (comment), steffahn outlined how to use this to, perhaps technically, trigger library UB using what is reported here. Just for our and my notes, here's what I think is a correct encoding of that:

Playground link

Note about earlier notifications.

(If you're following notifications on this thread, you may have seen I momentarily said some other things by way of analysis that turned out to not be correct and that I retracted soon after to try to minimize noise. Sorry about the noise there if you saw that; please disregard. Thought I was onto something; was not. Just was staring at it too long.)