Fix supertrait associated type unsoundness #126090

compiler-errors · 2024-06-06T18:47:56Z

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 #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 #126079

r? lcnr

compiler-errors · 2024-06-06T18:48:00Z

@bors try

…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

bors · 2024-06-06T18:49:11Z

⌛ Trying commit 3c44574 with merge 09fa131...

compiler-errors · 2024-06-06T19:02:31Z

compiler/rustc_trait_selection/src/traits/object_safety.rs

+                                    self.tcx.erase_regions(
+                                        self.tcx.instantiate_bound_regions_with_erased(trait_ref),
+                                    )


This sucks lol. but I don't really care about regions here

bors · 2024-06-06T20:26:18Z

☀️ Try build successful - checks-actions
Build commit: 09fa131 (09fa13185567a1d9c37905d6a4a2ea56e7f44efc)

compiler-errors · 2024-06-06T20:26:53Z

@craterbot check

craterbot · 2024-06-06T20:26:59Z

👌 Experiment pr-126090 created and queued.
🤖 Automatically detected try build 09fa131
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-06-07T05:36:54Z

🚧 Experiment pr-126090 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-06-09T06:02:14Z

🎉 Experiment pr-126090 is completed!
📊 11 regressed and 5 fixed (470015 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2024-07-02T19:46:55Z

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-126090/retry-regressed-list.txt

craterbot · 2024-07-02T19:46:59Z

👌 Experiment pr-126090-1 created and queued.
🤖 Automatically detected try build 09fa131
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-07-02T19:47:24Z

🚧 Experiment pr-126090-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-07-02T19:58:20Z

🎉 Experiment pr-126090-1 is completed!
📊 1 regressed and 0 fixed (11 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2024-07-02T20:33:47Z

This is ready. See description. Needs fcp.

lcnr · 2024-07-09T12:56:57Z

This fixes an unsoundness where a where-bound <Self as SuperTrait<DifferentArgFromSuperTraitBound>>::Assoc: Bound did not cause a trait to not be object safe. See the PR description and the added test.

@rfcbot fcp merge

r=me on the changes

rfcbot · 2024-07-09T12:56:59Z

Team member @lcnr 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.

nikomatsakis · 2024-07-15T17:52:18Z

@rfcbot reviewed

rfcbot · 2024-07-15T17:52:37Z

🔔 This is now entering its final comment period, as per the review above. 🔔

…aits

rfcbot · 2024-07-25T17:59:21Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

compiler-errors · 2024-07-25T18:01:14Z

@bors r=lcnr

bors · 2024-07-25T18:01:16Z

📌 Commit 172cf9b has been approved by lcnr

It is now in the queue for this repository.

…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

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

rustbot assigned lcnr Jun 6, 2024

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

compiler-errors commented Jun 6, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2024

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 9, 2024

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2024

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jul 2, 2024

compiler-errors added needs-fcp This change is insta-stable, so needs a completed 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

compiler-errors force-pushed the supertrait-assoc-ty-unsoundness branch from 3c44574 to ec21145 Compare July 2, 2024 20:33

compiler-errors marked this pull request as ready for review July 2, 2024 20:33

compiler-errors mentioned this pull request Jul 2, 2024

Item bounds can reference self projections and still be object safe #122804

Closed

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 9, 2024

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

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 15, 2024

compiler-errors added 2 commits July 15, 2024 14:16

Item bounds can reference self projections and still be object safe

8edf9b8

Fix unsoundness when associated types dont actually come from supertr…

172cf9b

…aits

compiler-errors force-pushed the supertrait-assoc-ty-unsoundness branch from ec21145 to 172cf9b Compare July 15, 2024 19:16

lcnr added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. and removed needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Jul 22, 2024

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

matthiaskrgr mentioned this pull request Jul 25, 2024

Rollup of 7 pull requests #128213

Merged

bors merged commit a883548 into rust-lang:master Jul 26, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Jul 26, 2024

apiraino removed the to-announce Announce this issue on triage meeting label Aug 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix supertrait associated type unsoundness #126090

Fix supertrait associated type unsoundness #126090

compiler-errors commented Jun 6, 2024 •

edited

Loading

compiler-errors commented Jun 6, 2024

bors commented Jun 6, 2024

compiler-errors Jun 6, 2024

This comment has been minimized.

bors commented Jun 6, 2024

compiler-errors commented Jun 6, 2024

craterbot commented Jun 6, 2024

craterbot commented Jun 7, 2024

craterbot commented Jun 9, 2024

compiler-errors commented Jul 2, 2024

craterbot commented Jul 2, 2024

craterbot commented Jul 2, 2024

craterbot commented Jul 2, 2024

compiler-errors commented Jul 2, 2024

lcnr commented Jul 9, 2024 •

edited

Loading

rfcbot commented Jul 9, 2024 •

edited

Loading

nikomatsakis commented Jul 15, 2024

rfcbot commented Jul 15, 2024

rfcbot commented Jul 25, 2024

compiler-errors commented Jul 25, 2024

bors commented Jul 25, 2024

Fix supertrait associated type unsoundness #126090

Fix supertrait associated type unsoundness #126090

Conversation

compiler-errors commented Jun 6, 2024 • edited Loading

What?

How do we fix it?

What is up w the stacked commit

compiler-errors commented Jun 6, 2024

bors commented Jun 6, 2024

compiler-errors Jun 6, 2024

Choose a reason for hiding this comment

This comment has been minimized.

bors commented Jun 6, 2024

compiler-errors commented Jun 6, 2024

craterbot commented Jun 6, 2024

craterbot commented Jun 7, 2024

craterbot commented Jun 9, 2024

compiler-errors commented Jul 2, 2024

craterbot commented Jul 2, 2024

craterbot commented Jul 2, 2024

craterbot commented Jul 2, 2024

compiler-errors commented Jul 2, 2024

lcnr commented Jul 9, 2024 • edited Loading

rfcbot commented Jul 9, 2024 • edited Loading

nikomatsakis commented Jul 15, 2024

rfcbot commented Jul 15, 2024

rfcbot commented Jul 25, 2024

compiler-errors commented Jul 25, 2024

bors commented Jul 25, 2024

compiler-errors commented Jun 6, 2024 •

edited

Loading

lcnr commented Jul 9, 2024 •

edited

Loading

rfcbot commented Jul 9, 2024 •

edited

Loading