CFI: Fix fn items, closures, and Fn trait objects #123082
Conversation
rustbot
added
PG-exploit-mitigations
|
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 |
There was a problem hiding this comment.
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) || is_fn_subtrait(tcx, predicates) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, it coalesces it under the first Fn supertrait found. How does #123106 handle it?
compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs
Outdated
Show resolved
Hide resolved
rcvalle
force-pushed
the
rust-cfi-fix-fn-items-closures-fn-trait-objects
branch
from
11b6387 to
faba20b
Compare
|
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 |
rcvalle
force-pushed
the
rust-cfi-fix-fn-items-closures-fn-trait-objects
branch
from
faba20b to
7c14164
Compare
Fix casting between function items, closures, and Fn trait objects by transforming function items, closures, and Fn trait objects into function pointers for encoding.
rcvalle
force-pushed
the
rust-cfi-fix-fn-items-closures-fn-trait-objects
branch
from
7c14164 to
70ac00e
Compare
|
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). |
|
☔ The latest upstream changes (presumably #123128) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
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.
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 |
rustbot
added
S-waiting-on-author
|
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). |
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