CFI: Skip non-passed arguments #122456
CFI: Skip non-passed arguments #122456
Conversation
rustbot
added
S-waiting-on-review
|
cc @rcvalle since this affects CFI in general as well. Can you cite an example of this reliance "in the wild"? For all I know there's already one in the rustc codebase, heh, just curious. |
|
The attached test is one such example - it reifies a closure into a function pointer by way of knowing that the closure context is a ZST. |
|
Huh, I didn't really think of the closure-to-fn-pointer decay that way, I suppose it makes sense. Can you add a note to the test that the ZST-ness of the captured context is part of what's relevant? |
| // CHECK: ![[TYPE137]] = !{i64 0, !"_ZTSFvu{{[0-9]+}}NtNvC{{[[:print:]]+}}_[[ITANIUMED_FILENAME]]3fn13QuxIu3i32Lu5usize32EES2_S2_E"} | ||
| // CHECK: ![[TYPE138]] = !{i64 0, !"_ZTSFvu{{[0-9]+}}NcNtNvC{{[[:print:]]+}}_[[ITANIUMED_FILENAME]]3fn15Quuux15{{[{}][{}]}}constructor{{[}][}]}}Iu6regionS_EE"} | ||
| // CHECK: ![[TYPE139]] = !{i64 0, !"_ZTSFvu{{[0-9]+}}NcNtNvC{{[[:print:]]+}}_[[ITANIUMED_FILENAME]]3fn15Quuux15{{[{}][{}]}}constructor{{[}][}]}}Iu6regionS_ES0_E"} | ||
| // CHECK: ![[TYPE140]] = !{i64 0, !"_ZTSFvu{{[0-9]+}}NcNtNvC{{[[:print:]]+}}_[[ITANIUMED_FILENAME]]3fn15Quuux15{{[{}][{}]}}constructor{{[}][}]}}Iu6regionS_ES0_S0_E"} |
There was a problem hiding this comment.
Why are you removing all these tests?
There was a problem hiding this comment.
These tests are being removed because their ABI coding becomes equivalent. You're passing in a ZST, so they all collapse.
There was a problem hiding this comment.
This doesn't seem right. We're losing a lot of granularity there. This seems to be the result of incorrectly removing the ZSTs vs unclosure'ing the Closures.
| //@ run-pass | ||
|
|
||
| pub fn main() { | ||
| let f: &fn() = &((|| ()) as _); |
There was a problem hiding this comment.
What happens if the closure has parameters?
There was a problem hiding this comment.
Then the cast is invalid:
pub fn main() {
let x: usize = 3;
let f: &fn() -> usize = &((|| x) as _);
}
generates
error[E0605]: non-primitive cast: `{closure@src/main.rs:3:32: 3:34}` as `fn() -> usize`
That cast is only legal for closures with no captures.
There was a problem hiding this comment.
I was referring to parameters, not captures.
There was a problem hiding this comment.
Why would that matter, exactly?
There was a problem hiding this comment.
I wanted to know how closures with parameters behave with these changes, but I guess considering all what I've seem and pointed out already this doesn't matter.
|
It seems this PR has an incomplete fix for a more general problem of allowing function items, closures, and Fn trait objects to be used interchangeably, which would then become a "complete" solution in your overall solution for KCFI with the addition of shims for it. As I mentioned in #121962 and to you directly a couple weeks ago, I'm currently working on a fix for this for both CFI and KCFI using type metadata IDs/encoding only (see #116404). which follows the same design of the rest of the CFI implementation by transforming/coalescing types we want to be semantically equivalent into a single entity and passing them for encoding, so the shims aren't necessary in this case even for KCFI. I'll resolve the merge conflicts and comments in #121962, confirm it fixes this issue, and try to get it merged next week if you don't mind waiting. If that doesn't fix this issue, we can have this merged (pending review). |
|
@rcvalle Does C++ actually have a coherent ABI for zero-sized arguments? If so, why is that a "more general solution" to fixing an incoherency in argument encoding? If C++ cannot represent it, why should it be encoded? If we do, we are getting awfully close to, assuming we haven't already passed, abandoning any pretense of actually using the Itanium mangling/C++ logic, and are just making up stuff. |
|
It's a more general solution for allowing function items, closures, and Fn trait objects to be used interchangeably that doesn't require shims. The ZSTs are gone in #116404 too when unclosure'ing the closures (see the else if is_closure_call in transform_fnabi there). |
|
@rcvalle If both erase ZSTs, then why not just land this first? |
|
Because this actually doesn't solve any problem completely or correctly. The problem isn't erasing ZSTs, the problem is properly unclosure'ing Closures and unwrapping dynamic Fn trait objects and encoding them correctly. This just partially fix the problem by erasing the ZSts from the Closures (instead of properly unclosure'ing them) for later "completing" the fix with shims/trampolines so they can be used interchangeably (also instead of encoding them correctly), which are not necessary. I just updated #116404 resolving the merge conflicts, fixing and doing a few things pointed out by @compiler-errors, and also added the test from this PR there to demonstrate that it not only fix this case, but more generally the problem I described before (see tests/codegen/sanitizer/cfi-emit-type-metadata-id-itanium-cxx-abi-closures.rs which illustrates it there). There are still a couple things left to do there that I'll take a look at next week. |
I have the complete solution in my full patch stack, but I was just asked to start splitting off smaller pieces which point-fixed some issues. If you want a full solution, look at my primary PR.
This is a separate problem from the ZSTs, and is addressed via the rewrite to |
|
It's the exact same problem, but you're solving it by adding shims instead. I mentioned this to you a couple weeks ago when we discussed your solution and I asked you to take a look and rebase your changes on top of it, eliminating these unnecessary shims, which you didn't. |
|
@rcvalle Setting aside the shim discussion, I am currently not persuaded that not erasing ZSTs is correct. All of the codegen backends erase ZSTs from fn arguments while emitting code, so why should the typechecking here be different? The languages we're concerned about checking indirect calls from/into don't even have ZSTs as a concept. |
|
I'm not saying that not erasing ZSTs is correct, I'm saying that just doing it is (and also how it's done manually vs unclosure'ing). Doing it is part of a larger issue that I already explained and repeated here many times: allowing function items, closures, and Fn trait objects to be used interchangeably (see tests/codegen/sanitizer/cfi-emit-type-metadata-id-itanium-cxx-abi-closures.rs which illustrates it). This PR doesn't solve it at all without adding unnecessary shims later. Just try tests/codegen/sanitizer/cfi-emit-type-metadata-id-itanium-cxx-abi-closures.rs with this PR without the rest of the changes that add the shims and you'll see). |
|
@rcvalle As this is an incremental improvement only, split as I requested, I don't see why @maurer would object to reverting these changes if a more complete fix was offered later, and I certainly wouldn't. I would hope that is acceptable to you, also? I realize that you would prefer shipping a more complete fix, but this entire work has been at least partly about feeling out better solutions and sometimes being surprised by what turns things take. |
|
...I mean, for me, my git-fu is pretty strong, so I'm just kinda assuming "rebase changes on top" isn't actually a big deal. |
Unless it's a subtree, tbh. |
|
☔ The latest upstream changes (presumably #122375) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I currently do not believe that it is possible for either form of CFI to realistically work without performing some ZST erasure. If it needs to be narrowed in the future from this, I feel that can be done later. @maurer Can you rebase this? Thanks! |
|
@maurer Also, when you do, please add a comment explaining that this ZST erasure is only sensical under |
|
If this is merged, it's going to decrease CFI granularity a lot. Basically, all groups of functions distinguished by ZSTs in Rust-compiled code will be gone. |
|
Some changes occurred in compiler/rustc_symbol_mangling/src/typeid cc @rust-lang/project-exploit-mitigations, @rcvalle 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 |
bors
added
S-waiting-on-author
|
@bors r+ |
bors
added
S-waiting-on-bors
CFI: Skip non-passed arguments Rust will occasionally rely on fn((), X) -> Y being compatible with fn(X) -> Y, since () is a non-passed argument. Relax CFI by choosing not to encode non-passed arguments. This PR was split off from rust-lang#121962 as part of fixing the larger vtable compatibility issues. r? `@workingjubilee`
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
The failure appears to be that OSX can't build CFI binaries. This patch doesn't adjust codegen, only labels, so if building is failing, it's an existing bug. It's one we should solve, but not a flaw in the patch. I'm going to adjust the test to be |
|
@maurer Please capture the PLT/reloc issue as a FIXME on the test. |
Rust will occasionally rely on fn((), X) -> Y being compatible with fn(X) -> Y, since () is a non-passed argument. Relax CFI by choosing not to encode non-passed arguments.
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (7762adc): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 667.777s -> 669.214s (0.22%) |
Rust will occasionally rely on fn((), X) -> Y being compatible with fn(X) -> Y, since () is a non-passed argument. Relax CFI by choosing not to encode non-passed arguments.
This PR was split off from #121962 as part of fixing the larger vtable compatibility issues.
r? @workingjubilee