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.

Sign up for GitHub

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

Jump to bottom

CFI: Fix cfi with async: transform_ty: unexpected GeneratorWitness(Bi… #111914

Merged
merged 1 commit into from
Jun 2, 2023
Merged

CFI: Fix cfi with async: transform_ty: unexpected GeneratorWitness(Bi… #111914

merged 1 commit into from
Jun 2, 2023

Conversation

rcvalle
Copy link
Member

@rcvalle rcvalle commented May 24, 2023 •
edited
Loading

Fixes #111184 by encoding ty::Generator parent substs only.

@rustbot
Copy link
Collaborator

rustbot commented May 24, 2023

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

@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 May 24, 2023
@rcvalle
Copy link
Member Author

rcvalle commented May 24, 2023

r? @bjorn3

@rustbot rustbot assigned bjorn3 and unassigned oli-obk May 24, 2023
@rcvalle rcvalle added the PG-exploit-mitigations Project group: Exploit mitigations label May 24, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member

bjorn3 commented May 24, 2023

Can't you encode ty::Generator as just the DefId + the parent_substs() part of the GeneratorSubsts? The witness and upvars should be uniquely determined by those two afaik.

@rcvalle rcvalle force-pushed the rust-cfi-fix-111184 branch from 0c479d8 to ff03d7f Compare May 24, 2023 18:20
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rcvalle rcvalle force-pushed the rust-cfi-fix-111184 branch from ff03d7f to 38ff85b Compare May 25, 2023 20:47
@bjorn3
Copy link
Member

bjorn3 commented May 25, 2023

@compiler-errors do you think the new implementation makes sense?

@bjorn3 bjorn3 requested a review from compiler-errors May 25, 2023 20:54
@rcvalle rcvalle force-pushed the rust-cfi-fix-111184 branch from 38ff85b to 8331590 Compare May 25, 2023 20:57
compiler-errors
compiler-errors reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this doesn't implement either mine or @bjorn3's suggestions, does it? I think we need to split out the branch for ty::Generator in encode_ty, and either:

  • As bjorn3 suggested, only encode the def-id and parent substs
  • As I suggested, "skip" the generator witness by destructuring the generator witness so we never encounter ty::GeneratorWitness in encode_ty at all.

@compiler-errors
Copy link
Member

For the record, @bjorn3's suggestion is probably easier to implement. Generators can't be polymorphic so they are totally uniquely determined by their def path + parent substs.

@rcvalle
Copy link
Member Author

rcvalle commented Jun 1, 2023 •
edited
Loading

I think it implements @bjorn3's suggestion by just skipping encoding ty::GeneratorWitness, which will happen only as a ty::Generator substitution, but the parent DefId and substitutions (other than the ty::GeneratorWitness) will be encoded when encoding the ty::Generator itself. Does it sound right to you?

@compiler-errors
Copy link
Member

No, that's not exactly right. You need to get the generator substs and explicitly extract just the parent substitutions from that.

substs.as_generator().split().parent_substs and then pass that to encode_substs.

The current implementation is still encoding all the other substs except for the witness in GeneratorSubstsParts (https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/sty/struct.GeneratorSubstsParts.html#structfield.resume_ty).

@rcvalle
Copy link
Member Author

rcvalle commented Jun 1, 2023

Oh, I see. There are other substitutions besides the parent and the witness, and skipping just the witness will still include other substitutions besides the parent that we don't want. Thanks for pointing it out! I'll fix it.

@rcvalle rcvalle force-pushed the rust-cfi-fix-111184 branch 4 times, most recently from 4ab15c2 to 308cd80 Compare June 1, 2023 22:19
@rcvalle
Copy link
Member Author

rcvalle commented Jun 1, 2023

Done. Let me know if it looks good to you!

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rcvalle rcvalle force-pushed the rust-cfi-fix-111184 branch from 308cd80 to 76ff5ec Compare June 1, 2023 23:23
@rcvalle
Copy link
Member Author

rcvalle commented Jun 2, 2023

Thank you again for your time and for your help on this, @compiler-errors! Much appreciated.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2023

📌 Commit 76ff5ec has been approved by compiler-errors

It is now in the queue for this repository.

@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 Jun 2, 2023
@compiler-errors compiler-errors mentioned this pull request Jun 2, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 2, 2023
@bors
Auto merge of rust-lang#112198 - compiler-errors:rollup-o2xe4of, r=co…
8ebf042 
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#111670 (Require that const param tys implement `ConstParamTy`)
 - rust-lang#111914 (CFI: Fix cfi with async: transform_ty: unexpected GeneratorWitness(Bi…)
 - rust-lang#112030 (Migrate `item_trait_alias` to Askama)
 - rust-lang#112150 (Support 128-bit atomics on all x86_64 Apple targets)
 - rust-lang#112174 (Fix broken link)
 - rust-lang#112190 (Improve comments on `TyCtxt` and `GlobalCtxt`.)
 - rust-lang#112193 (Check tuple elements are `Sized` in `offset_of`)

Failed merges:

 - rust-lang#112071 (Group rfcs tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ceec225 into rust-lang:master Jun 2, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 2, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jun 2, 2023
Open
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 30, 2023
@bors
Auto merge of rust-lang#112198 - compiler-errors:rollup-o2xe4of, r=co…
427a453 
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#111670 (Require that const param tys implement `ConstParamTy`)
 - rust-lang#111914 (CFI: Fix cfi with async: transform_ty: unexpected GeneratorWitness(Bi…)
 - rust-lang#112030 (Migrate `item_trait_alias` to Askama)
 - rust-lang#112150 (Support 128-bit atomics on all x86_64 Apple targets)
 - rust-lang#112174 (Fix broken link)
 - rust-lang#112190 (Improve comments on `TyCtxt` and `GlobalCtxt`.)
 - rust-lang#112193 (Check tuple elements are `Sized` in `offset_of`)

Failed merges:

 - rust-lang#112071 (Group rfcs tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@rcvalle rcvalle deleted the rust-cfi-fix-111184 branch April 22, 2024 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb left review comments

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@bjorn3 bjorn3

Labels
PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.72.0
Development

Successfully merging this pull request may close these issues.

ICE: cfi with async: transform_ty: unexpected GeneratorWitness(Binder..
8 participants
@rcvalle @rustbot @rust-log-analyzer @bjorn3 @compiler-errors @bors @Noratrieb @oli-obk