Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Associated types in object-safe method signatures don't always come from supertraits #126079

Closed
compiler-errors opened this issue Jun 6, 2024 · 0 comments · Fixed by #126090
Closed
Assignees
Labels
A-trait-objects Area: trait objects, vtable layout C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@compiler-errors
Copy link
Member

compiler-errors commented Jun 6, 2024

One of the requirements for a trait to be object-safe is that the associated types that show up in signatures of methods come from the supertraits of the object. We don't currently implement this correctly -- we currently ignore the substitutions of the associated type 😿.

This is unsound:

use core::marker::PhantomData;

fn transmute<T, U>(t: T) -> U {
    (&PhantomData::<T> as &dyn Foo<T, U>).transmute(t)
}

struct ActuallySuper;
struct NotActuallySuper;
trait Super<Q> {
    type Assoc;
}

trait Dyn {
    type Out;
}
impl<T, U> Dyn for dyn Foo<T, U> + '_ {
    type Out = U;
}
impl<S: Dyn<Out = U> + ?Sized, U> Super<NotActuallySuper> for S {
    type Assoc = U;
}

trait Foo<T, U>: Super<ActuallySuper, Assoc = T> where <Self as Mirror>::Assoc: Super<NotActuallySuper> {
    fn transmute(&self, t: T) -> <Self as Super<NotActuallySuper>>::Assoc;
}

trait Mirror {
    type Assoc: ?Sized;
}
impl<T: ?Sized> Mirror for T {
    type Assoc = T;
}

impl<T, U> Foo<T, U> for PhantomData<T> {
    fn transmute(&self, t: T) -> T {
        t
    }
}
impl<T> Super<ActuallySuper> for PhantomData<T> {
    type Assoc = T;
}
impl<T> Super<NotActuallySuper> for PhantomData<T> {
    type Assoc = T;
}

fn main() {
    let x = String::from("hello, world");
    let s = transmute::<&str, &'static str>(x.as_str());
    drop(x);
    println!("> {s}");
}

Specifically, fn transmute(&self, t: T) -> <Self as Super<NotActuallySuper>>::Assoc; should only be sound if the return type was <Self as Super<ActuallySuper>>::Assoc, or if we added Super<NotActuallySuper> as a supertrait.

We should probably deeply normalize these supertraits and actually consider the associated types' substs match modulo regions.

@compiler-errors compiler-errors added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-types Relevant to the types team, which will review and decide on the PR/issue. labels Jun 6, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 6, 2024
@compiler-errors compiler-errors self-assigned this Jun 6, 2024
@fmease fmease added C-bug Category: This is a bug. A-trait-objects Area: trait objects, vtable layout and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 6, 2024
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 7, 2024
@lcnr lcnr added the P-high High priority label Jun 10, 2024
@lcnr lcnr moved this to unblocked in T-types unsound issues Jun 10, 2024
@bors bors closed this as completed in a883548 Jul 26, 2024
@github-project-automation github-project-automation bot moved this from unblocked to new solver everywhere in T-types unsound issues Jul 26, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 26, 2024
Rollup merge of rust-lang#126090 - compiler-errors:supertrait-assoc-ty-unsoundness, r=lcnr

Fix supertrait associated type unsoundness

### What?

Object safety allows us to name `Self::Assoc` associated types in certain positions if they come from our trait or one of our supertraits. When this check was implemented, I think it failed to consider that supertraits can have different args, and it was only checking def-id equality.

This is problematic, since we can sneak different implementations in by implementing `Supertrait<NotActuallyTheSupertraitSubsts>` for a `dyn` type. This can be used to implement an unsound transmute function. See the committed test.

### How do we fix it?

We consider the whole trait ref when checking for supertraits. Right now, this is implemented using equality *without* normalization. We erase regions since those don't affect trait selection.

This is a limitation that could theoretically affect code that should be accepted, but doesn't matter in practice -- there are 0 crater regression. We could make this check stronger, but I would be worried about cycle issues. I assume that most people are writing `Self::Assoc` so they don't really care about the trait ref being normalized.

---

### What is up w the stacked commit

This is built on top of rust-lang#122804 though that's really not related, it's just easier to make this modification with the changes to the object safety code that I did in that PR. The only thing is that PR may make this unsoundness slightly easier to abuse, since there are more positions that allow self-associated-types -- I am happy to stall that change until this PR merges.

---

Fixes rust-lang#126079

r? lcnr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-objects Area: trait objects, vtable layout C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants