CFI: Fix fn items, closures, and Fn trait objects by rcvalle · Pull Request #123082 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fix casting between function items, closures, and Fn trait objects by transforming function items, closures, and Fn trait objects into function pointers for encoding.
This was split off from #116404.
cc @compiler-errors @workingjubilee
r? @lcnr
rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Some changes occurred in tests/codegen/sanitizer
cc @rust-lang/project-exploit-mitigations, @rcvalle
Some changes occurred in tests/ui/sanitizer
cc @rust-lang/project-exploit-mitigations, @rcvalle
Some changes occurred in compiler/rustc_symbol_mangling/src/typeid
cc @rust-lang/project-exploit-mitigations, @rcvalle
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review not yet complete, heading out to something else but wanted to leave what comments I had so far.
My biggest concern is how deeply this needs to adjust our type encoding to deal with closures and Fn traits, and the number of epicycles it seems to add. I'll extend the review later tonight or tomorrow morning.
| tcx.lifetimes.re_erased, |
|---|
| *kind, |
| ); |
| if is_fn_trait(tcx, predicates) | |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would only be possible with #[feature(fn_traits)], and doesn't seem likely in user code, but doesn't this mean that something like this:
trait CountedCallable: Fn(i32) {
fn times_called(&self) -> usize;
}
struct Foo;
impl Fn<(i32)> for Foo { /* .. */ }
impl CountedCallable for Foo { /* .. */ }
then Foo::times_called would have an encoded alias set of fn(fn(i32)) -> usize? Is that intended?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but your alternative at #123106, would require attaching either as the main or secondary type id fn(&dyn Fn(i32)) -> usize as well, right?
| return Some(args[0].expect_ty().tuple_fields()); |
|---|
| } |
| if is_fn_subtrait(tcx, predicates) { |
| return Some(List::empty()); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right. Two problems I see here:
- Does this mean that you're encoding the args as empty regardless of what the trait implements?
- A trait can have multiple fn traits as supertraits, e.g.
trait Foo: Fn() + Fn(i32) -> u8 {}. It's weird, but since you're implementing this it seems worth discussing.
These same questions stand for fn_trait_output.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it coalesces it under the first Fn supertrait found. How does #123106 handle it?
Some changes occurred in tests/codegen/sanitizer
cc @rust-lang/project-exploit-mitigations, @rcvalle
Some changes occurred in compiler/rustc_symbol_mangling/src/typeid
cc @rust-lang/project-exploit-mitigations, @rcvalle
Some changes occurred in tests/ui/sanitizer
cc @rust-lang/project-exploit-mitigations, @rcvalle
Fix casting between function items, closures, and Fn trait objects by transforming function items, closures, and Fn trait objects into function pointers for encoding.
We're moving forward with #123106, which is a variant of this (that transforms closures into dynamic Fn traits but isolating it to the Fn call methods only). See my comment on #123106 (comment).
rcvalle added a commit to rcvalle/rust that referenced this pull request
Since we're now using an approach which is a variant of rust-lang#123082 (that transforms closures into dynamic Fn traits but isolating it to the Fn call methods only) instead of rust-lang#121962 or rust-lang#122573, skipping non-passed arguments shouldn't be necessary KCFI anymore and we can claim back the reduced granularity.
This reverts commit f2f0d25.
rcvalle added a commit to rcvalle/rust that referenced this pull request
Since we're now using an approach which is a variant of rust-lang#123082 (that transforms closures into dynamic Fn traits but isolating it to the Fn call methods only) instead of rust-lang#121962 or rust-lang#122573, skipping non-passed arguments shouldn't be necessary for KCFI anymore and we can claim back the reduced granularity.
This reverts commit f2f0d25.
maurer pushed a commit to maurer/rust that referenced this pull request
Since we're now using an approach which is a variant of rust-lang#123082 (that transforms closures into dynamic Fn traits but isolating it to the Fn call methods only) instead of rust-lang#121962 or rust-lang#122573, skipping non-passed arguments shouldn't be necessary for KCFI anymore and we can claim back the reduced granularity.
This reverts commit f2f0d25.
this PR is very stale i think
@rustbot author
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Changed back to draft. Thank you!
@rcvalle do you still need this pr since the other is merged? if so, do you have any updates on this? thanks
I'm still considering an improved version of this. The alternative has had a lot of bugs/churn and required regressions that so far couldn't be reverted (see #123205).
Labels
Project group: Exploit mitigations
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the compiler team, which will review and decide on the PR/issue.