Fix supertrait associated type unsoundness by compiler-errors · Pull Request #126090 · rust-lang/rust

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Jun 6, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jun 6, 2024
…unsoundness, r=<try>

Fix supertrait associated type unsoundness

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.

This PR only looks for supertrait associated types *without* normalizing. I'm gonna crater this initially -- preferably we don't need to normalize unless there's a lot of breakage. I assume that most people are writing `Self::Assoc` so they don't really care about the trait ref being normalized, so somewhat tempted to believe that this will just work out and we can extend it later if needed.

r? lcnr

compiler-errors

@compiler-errors compiler-errors added needs-fcp

This change is insta-stable, or significant enough to need a team FCP to proceed.

T-types

Relevant to the types team, which will review and decide on the PR/issue.

and removed T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Jul 2, 2024

@rfcbot rfcbot added the final-comment-period

In the final comment period and will be merged soon unless new substantive objections are raised.

label

Jul 15, 2024

@compiler-errors

@compiler-errors

@lcnr lcnr added needs-fcp

This change is insta-stable, or significant enough to need a team FCP to proceed.

and removed needs-fcp

This change is insta-stable, or significant enough to need a team FCP to proceed.

labels

Jul 22, 2024

@bors bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jul 25, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 26, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#126090 (Fix supertrait associated type unsoundness)
 - rust-lang#127220 (Graciously handle `Drop` impls introducing more generic parameters than the ADT)
 - rust-lang#127950 (Use `#[rustfmt::skip]` on some `use` groups to prevent reordering.)
 - rust-lang#128085 (Various notes on match lowering)
 - rust-lang#128150 (Stop using `unsized_const_parameters` in core/std)
 - rust-lang#128194 (LLVM: LLVM-20.0 removes MMX types)
 - rust-lang#128211 (fix: compilation issue w/ refactored type)

r? `@ghost`
`@rustbot` modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

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