Make casts of pointers to trait objects stricter #120248
Conversation
|
r? @TaKO8Ki (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
| @@ -100,7 +100,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
|
|
|||
| Ok(match *t.kind() { | |||
| ty::Slice(_) | ty::Str => Some(PointerKind::Length), | |||
| ty::Dynamic(tty, _, ty::Dyn) => Some(PointerKind::VTable(tty.principal_def_id())), | |||
| ty::Dynamic(tty, _, ty::Dyn) => Some(PointerKind::VTable(tty.principal())), | |||
There was a problem hiding this comment.
👍 Ah, ok, so this is fine because we erase regions in check_ptr_ptr_cast.
|
@bors try |
…r=<try> Make casts of pointers to trait objects stricter This is an attempt to `fix` rust-lang#120222 and rust-lang#120217. cc `@oli-obk` `@compiler-errors` `@lcnr`
This comment has been minimized.
This comment has been minimized.
| if let ty::RawPtr(src_pointee) = self.expr_ty.kind() | ||
| && let ty::RawPtr(tgt_pointee) = self.cast_ty.kind() | ||
| { | ||
| if let Ok(Some(src_kind)) = fcx.pointer_kind(src_pointee.ty, self.expr_span) | ||
| && let Ok(Some(tgt_kind)) = | ||
| fcx.pointer_kind(tgt_pointee.ty, self.cast_span) | ||
| { | ||
| match (src_kind, tgt_kind) { | ||
| // When casting a raw pointer to another raw pointer, we cannot convert the cast into | ||
| // a coercion because the pointee types might only differ in regions, which HIR typeck | ||
| // cannot distinguish. This would cause us to erroneously discard a cast which will | ||
| // lead to a borrowck error like #113257. | ||
| // We still did a coercion above to unify inference variables for `ptr as _` casts. | ||
| // This does cause us to miss some trivial casts in the trivial cast lint. | ||
| (PointerKind::Thin, PointerKind::Thin) | ||
| | (PointerKind::Length, PointerKind::Length) => { | ||
| debug!(" -> PointerCast"); | ||
| } | ||
|
|
||
| // If we are not casting pointers to sized types or slice-ish DSTs | ||
| // (handled above), we need to make a coercion cast. This prevents | ||
| // casts like `*const dyn Trait<'a> -> *const dyn Trait<'b>` which | ||
| // are unsound. | ||
| // | ||
| // See <https://github.com/rust-lang/rust/issues/120217> | ||
| (_, _) => { | ||
| debug!(" -> CoercionCast"); | ||
| fcx.typeck_results | ||
| .borrow_mut() | ||
| .set_coercion_cast(self.expr.hir_id.local_id); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This will only work for pointers that can be coerced modulo regions, it will still let something like this pass:
trait Trait<'a> {}
fn cast<'a, 'b>(x: *mut dyn Trait<'a>) -> *mut (dyn Trait<'b> + Send) {
x as _
}I think the correct fix would be to actually borrow check the principals for equality around here:
Also, extending the trait object lifetime itself (e.g. *mut dyn Trait<'a> + 'a -> *mut dyn Trait<'a> + 'static) is harmless and should be allowed, just like adding auto-traits is.
Edit: borrowck version here: c471b9b
There was a problem hiding this comment.
something like this pass:
It does indeed let it pass, but why? These are not coercible?..
I think the correct fix would be to actually borrow check the principals for equality around here:
I also thought of that, but it seemed somewhat painful to do. And since we handle upcasts with CastKind::PointerCoercion, this seemed like a good solution. Borrowck change might be the right solution after all though.
Edit: borrowck version here: c471b9b
This won't help with *mut (u8, dyn Trait<'a>) -> *mut (u8, dyn Trait<'b>), but that should be an easy fix with struct_tail...
Also, extending the trait object lifetime itself (e.g. *mut dyn Trait<'a> + 'a -> *mut dyn Trait<'a> + 'static) is harmless and should be allowed, [...]
Hm, I can't find an example why it isn't harmless, but I also can't really convince myself that it is. Is this something to do with the fact that you can given dyn Trait<'a> + 'b you can use 'a in Trait's method's signatures, but not 'b?
[...], harmless and should be allowed, just like adding auto-traits is.
Adding auto-traits is not harmless. Here is an example that segfaults in safe code with arbitraty self receivers and adding send:
example
#![feature(arbitrary_self_types)]
trait Trait {
fn f(self: *const Self)
where
Self: Send;
}
impl Trait for *const () {
fn f(self: *const Self) {
unreachable!()
}
}
fn main() {
let unsend: *const dyn Trait = &(&() as *const ());
let send_bad: *const (dyn Trait + Send) = unsend as _;
send_bad.f();
}fish: Job 1, './t' terminated by signal SIGSEGV (Address boundary error)
(play)
Notably, even though we had to implement f, it's not actually in a vtable, since you should not be able to call it (because of unsatisfied Send bound):
vtable layout
error: vtable entries for `<*const () as Trait>`: [
MetadataDropInPlace,
MetadataSize,
MetadataAlign,
Vacant,
]
--> ./t.rs:24:1
|
24 | trait Trait {
| ^^^^^^^^^^^
There was a problem hiding this comment.
Adding auto-traits is not harmless. Here is an example that segfaults in safe code with arbitraty self receivers and adding send:
Interesting. I was somehow convinced these functions were still codegened and thought it'd be harmless because you can't do anything bad with with a sendable raw pointer alone.
This also means that borrow check alone won't be enough and we need to treat these casts more like coercions.
Is this something to do with the fact that you can given
dyn Trait<'a> + 'byou can use'ainTrait's method's signatures, but not'b?
Yeah that was my reasoning as well. You can add bounds like Self: 'static, but since the maybe-not-'static thing is behind a raw pointer you shouldn't be able to do anything bad with it. But that same reasoning was already wrong for auto traits so I'm not sure anymore.
Also, extending the lifetime of trait objects is often used in unsafe code (like the one in std), so forbidding this will probably cause widespread breakage.
something like this pass:
It does indeed let it pass, but why? These are not coercible?..
What I'm trying to say is that because these are not coercible, the try_coercion_cast above will fail, which means that the cast will be a CastKind::PtrToPtr and not a coercion. And CastKind::PtrToPtr is absolutely not borrow checked so any lifetime can be cast to any other lifetime.
There was a problem hiding this comment.
This also means that borrow check alone won't be enough and we need to treat these casts more like coercions.
I think that we can check that in hir_typeck, the same way we check
let x: &dyn Trait + Send = &();
let y: &dyn Trait = x;(I think this is done in hir_typeck? will have to find out...)
What I'm trying to say is that because these are not coercible, the
try_coercion_castabove will fail, which means that the cast will be aCastKind::PtrToPtrand not a coercion. AndCastKind::PtrToPtris absolutely not borrow checked so any lifetime can be cast to any other lifetime.
Ah, I see. Right.
Do you think moving the check out of try_coercion_cast's match would fix the issue (possibly always reporting an error if try_coercion_cast fails for *mut dyn ...)? Or should I move to your proposed solution with doing things for PtrToPtr in borrowck?
There was a problem hiding this comment.
Do you think moving the check out of
try_coercion_cast'smatchwould fix the issue (possibly always reporting an error iftry_coercion_castfails for*mut dyn ...)?
Where exactly do you want to move it? If this is checked unconditionally even after do_check, then all ptr-ptr casts with vtables that can't be coerced will be just be forbidden, right? Like casting *mut Wrapper<dyn Trait> <-> *mut dyn Trait.
I think that we can check that in
hir_typeck, the same way we checklet x: &dyn Trait + Send = &(); let y: &dyn Trait = x;
I think checking this here in check_ptr_ptr_cast where we also compare the principals might be fine. Just check that the source includes all auto traits from the target.
And I'm pretty sure we'd still need borrowck for the principal on top of that.
There was a problem hiding this comment.
Checking this with the Unsize looks risky to me. That trait is also implemented for trait upcasts, e.g. dyn Subtrait: Unsize<dyn Supertrait>, because trait upcasting is essentially treated as a "unsizing" coercion before codegen, except that the source type is already unsized.
Because of that, with your current implementation this will compile:
trait Super {}
trait Sub: Super {}
struct Wrapper<T: ?Sized>(T);
fn cast(ptr: *const dyn Sub) -> *const Wrapper<dyn Super> {
// this is a "reinterpret cast" and not an upcast, the vtable is just copied
ptr as _
}This could theoretically be fine, because it looks like there is currently no (safe) way to cast from *const Wrapper<dyn Super> to *const dyn Super. This happens, because try_coercion_cast will (via Coerce::coerce_unsized) eagerly evaluate the CoerceUnsized and Unsize obligations, but not their nested obligations, which means that casting *const Wrapper<dyn Super> to *const dyn Super is treated as a coercion and fails. But this also leads to weird inconsistencies whether a cast is considered a coercion cast or pointer-pointer cast.
example
trait Trait {}
struct Wrapper<T: ?Sized>(T);
fn cast1(x: *const dyn Send) -> *const (dyn Send + Sync) {
x as _ // is a ptr-ptr cast, compiles
}
fn cast2(x: *const Wrapper<dyn Send>) -> *const (dyn Send + Sync) {
x as _ // is an unsizing coercion cast, error
}So I'd argue the current behavior is a bug and it should always be allowed to cast *const Wrapper<dyn Trait> to *const dyn Trait, which would make your implementation incorrect.
There was a problem hiding this comment.
*const dyn Sub -> *const Wrapper<dyn Super> being a "reinterpret cast" is very bad, for the same reasons we have the issues this PR attempts to fix — having an incorrect vtable is not sound with other features. I'll add back the check that the principal traits are the same. While I don't see a safe way to go *const W<dyn Trait> -> *const dyn Trait, this transformation looks more than fine for me, so I really don't want to depend on it being impossible. (tbh I assumed it was possible, considering you can do *const dyn Trait -> *const W<dyn Trait>, it's very weird we don't allow the opposite...)
So I'd argue the current behavior is a bug and it should always be allowed to cast
*const Wrapper<dyn Trait>to*const dyn Trait, which would make your implementation incorrect.
In other words: I agree with this.
Although I'm not sure how you can add this in a non-confusing way.
*const ()->*const Sendmust be unsizing*const W<dyn Trait>->*const dyn Traitshould be ptr-ptr- But what if
W<dyn Trait>: Trait? Currently it's unsizing, do we keep that?
- But what if
There was a problem hiding this comment.
- But what if
W<dyn Trait>: Trait? Currently it's unsizing, do we keep that?
*const W<dyn Trait> -> *const dyn Trait can't be an unsizing cast, because W<dyn Trait> is not sized and not a trait object. We can't keep the vtable metadata that is already in the fat pointer *const W<dyn Trait> and add the metadata of an impl Trait for W<dyn Trait> on top of that.
Casting *const T to *const U should be an unsizing coercion if and only if T: Unsize<U>, but W<dyn Trait> can never implement Unsize<dyn Trait>.
The problem with the current implementation is that it doesn't fully check W<dyn Trait>: Unsize<dyn Trait>. It only sees that W<dyn Trait>: Unsize<dyn Trait> holds if the nested obligations W<dyn Trait>: Sized and W<dyn Trait>: Trait hold but then directly assumes the cast must be an unsizing coercion and only checks the nested obligations too late.
There was a problem hiding this comment.
Oooh, right.
Yeah, then *const W<dyn Trait> -> *const dyn Trait should definitely be supported, but maybe as a follow up to this :)
There was a problem hiding this comment.
It is possible to do *const W<dyn Trait> -> *const dyn Trait right now in safe code. You just need to write a small helper function
impl<T: ?Sized> Wrapper<T> {
fn get(this: *const Self) -> *const T {
this as _
}
}
fn cast(ptr: *const Wrapper<dyn Super>) -> *const dyn Super {
Wrapper::get(ptr)
}
|
☀️ Try build successful - checks-actions |
|
@rustbot author |
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
|
not following this pr too closely for now 🙇 afaict there are some challenges in getting this to work correctly. wanted to quickly chime in that i believe at some point #120222 (comment) to be preferable simply because it's easier to confirm that it is actually correct.
for #120217 we could similarly simply forbid calling raw-ptr methods on trait objects/not making them object safe. |
The rules for casting `*mut X<dyn A>` -> `*mut Y<dyn B>` are as follows: - If `B` has a principal - `A` must have exactly the same principal (including generics) - Auto traits of `B` must be a subset of autotraits in `A` Note that `X<_>` and `Y<_>` can be identity, or arbitrary structs with last field being the dyn type. The lifetime of the trait object itself (`dyn ... + 'a`) is not checked. This prevents a few soundness issues with `#![feature(arbitrary_self_types)]` and trait upcasting. Namely, these checks make sure that vtable is always valid for the pointee.
rust-cloud-vms
bot
force-pushed
the
bonk-ptr-object-casts
branch
from
3c3cf17
to
5f27951
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors try |
…r=<try> Make casts of pointers to trait objects stricter This is an attempt to `fix` rust-lang#120222 and rust-lang#120217. This is done by adding restrictions on casting pointers to trait objects. Currently the restriction is > When casting `*const X<dyn A>` -> `*const Y<dyn B>`, if `B` has a principal trait, `dyn A + 'erased: Unsize<dyn B + 'erased>` must hold This ensures that 1. Principal trait's generic arguments match (no `*const dyn Tr<A>` -> `*const dyn Tr<B>` casts, which are a problem for [rust-lang#120222](rust-lang#120222)) 2. Principal trait's lifetime arguments match (no `*const dyn Tr<'a>` -> `*const dyn Tr<'b>` casts, which are a problem for [rust-lang#120217](rust-lang#120217)) 3. No auto traits can be _added_ (this is a problem for arbitrary self types, see [this comment](rust-lang#120248 (comment))) Some notes: - We only care about the metadata/last field, so you can still cast `*const dyn T` to `*const WithHeader<dyn T>`, etc - The lifetime of the trait object itself (`dyn A + 'lt`) is not checked, so you can still cast `*mut FnOnce() + '_` to `*mut FnOnce() + 'static`, etc - This feels fishy, but I couldn't come up with a reason it must be checked - The new checks are only done if `B` has a principal, so you can still do any kinds of cast, if the target only has auto traits - This is because auto traits are not enough to get unsoundness issues that this PR fixes - ...and so it makes sense to minimize breakage The plan is to, ~~once the checks are properly implemented~~, run crate to determine how much damage does this do. The diagnostics are currently not great, to say the least, but as far as I can tell this correctly fixes the issues. cc `@oli-obk` `@compiler-errors` `@lcnr`
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
Crater Reportanymap 1.0The most impactful regression is in
other regressions
|
|
I'm nominating this for discussion in t-lang. I want to confirm if we are OK with the following breaking changes. For context, those are being made to prevent a few different, yet related, unsound issues found while stabilizing trait upcasting. Those issues require either The issues are #120222 and #120217. There is also an issue found while working on this PR, which has not been opened as a separate issue. This comment will refer to specific issues/comments/examples when they are relevant to the proposed breaking changes. See also @lcnr's comment down below for an alternative solution. 1. Generics must matchThis prevents casting There is only one crater failure due to this (and the crate is not dependent on by anyone) (see As a side-note: we already prevent casting 2. Lifetimes must matchThis prevents casting There is only one crater failure (+2 dependants) (see Note that casting 3. Auto traits can't be addedThis prevents casting #![feature(arbitrary_self_types)]
trait Trait {
fn f(self: *const Self)
where
Self: Send;
}
impl Trait for *const () {
fn f(self: *const Self) {
unreachable!()
}
}
fn main() {
let unsend: *const dyn Trait = &(&() as *const ());
let send_bad: *const (dyn Trait + Send) = unsend as _;
send_bad.f(); // f is not actually in the vtable, since `*const (): Send` bound is not satisfied
}This causes the most breakage (see above crater report). ConclusionI believe 1 and 2 should be done. They don't seem to cause almost any breakage & generally make sense (if we want the "pointer vtables are correct" and stabilize trait upcasting). I'm not sure what to do with the 3-rd change though. It causes noticeable breakage. Although most of it is in an unmaintained crate last version of which is unsound anyway, so maybe we can still do this change (after writing a replacement for anymap and sending patches to all affected dependents)? If we want to prevent immediate breakage for the auto trait change we can try downgrading errors to FCWs in sound cases (i.e. where the trait does not have bounds with the autotrait) and marking making them into errors as a blocker for arbitrary self types. However, implementing this seems tricky. We could also decide not not error in sound cases at all (+optionally issue a normal warning), but that would make "adding a method with an auto-trait bound" into a technically breaking change. (I'm not saying this is a bad way) |
WaffleLapkin
added
the
I-lang-nominated
|
For context, the alternative approach to fixing the soundness issues is by 1) making vtables less efficient by storing entries even if they should be unnecessary, preventing the casts from changing their layout this way and 2) restricting |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as resolved.
This comment was marked as resolved.
WaffleLapkin
added
T-lang
|
@rfcbot fcp merge We discussed this in the @rust-lang/lang meeting today and meeting consensus was to do the following:
We are not opposed to a more precise check that would accept anymap but it in general would be better to just disallow this if we can. The anymap crate is a persistent problem because it hits the trifecta: leans on unsafe magic, unmaintained, and fills a real need and hence gets actual use. Rather than bending over backwards to accommodate it, we wanted to raise the possibility of taking it over somehow or issuing a new anymap crate, maybe adding it into the stdlib, something that will put us in a better position longer term. |
|
Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
WaffleLapkin
added
T-compiler
To be clear, this is the restriction after this PR, not the one on current master? Could you contrast this with the restriction before this PR, if any? |
|
@RalfJung currently we only require that trait def ids are matching (allowing |
|
...regarding:
(Chris, if you see this and are OK with the Rust Project adopting or co-maintaining See also: |
rfcbot
added
the
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
removed
the
proposed-final-comment-period
|
@rfcbot reviewed I'm okay with the steps outlined in this FCP.
This seems okay; all the use cases I can think of are satisfied with
I think both of these would be entirely reasonable if we had some use case for manipulating pointers to trait objects in this way. For example, maybe we want to allow unsafe code to do something like the following: let ptr = &() as *const dyn Foo + Send;
let ptr = ptr as *const dyn Foo; // with more regular vtables, this would be safe
// store ptr somewhere.
// ..later..
// Notice that "downcasting" a trait object pointer is unsafe.
// SAFETY: Okay because if we got here, the original pointer and vtable are Send.
let ptr = unsafe { ptr as *const dyn Foo + Send };If we continue with the breaking changes in this FCP, it might be possible to allow this while also allowing raw pointers to trait objects as receivers of safe methods. Either way, we should keep both of these unstable until a compelling use case is presented for them. |
This is an attempt to
fix#120222 and #120217.This is done by adding restrictions on casting pointers to trait objects. Currently the restriction in this PR is
As opposed to the stable restriction of
This ensures that
*const dyn Tr<A>->*const dyn Tr<B>casts, which are a problem for #120222)*const dyn Sub->*const dyn SupMake casts of pointers to trait objects stricter #120248 (comment)*const dyn Tr<'a>->*const dyn Tr<'b>casts, which are a problem for #120217)Some notes:
*const dyn Tto*const WithHeader<dyn T>, etcdyn A + 'lt) is not checked, so you can still cast*mut FnOnce() + '_to*mut FnOnce() + 'static, etcBhas a principal, so you can still do any kinds of cast, if the target only has auto traitsThe plan is to,
once the checks are properly implemented, run crater to determine how much damage does this do.The diagnostics are currently not great, to say the least, but as far as I can tell this correctly fixes the issues.
cc @oli-obk @compiler-errors @lcnr