Repository navigation
CFI: Cover each InstanceKind explicitly - #160230
Conversation
rustbot
added
A-tidy
rcvalle
added
the
A-control-flow-integrity
a7acb1a to
b8945d8
Compare
This comment has been minimized.
This comment has been minimized.
|
The first commit does both mechanical refactorings and non-refactoring changes together. Would you mind splitting the variable renamings and outlining into a separate commit to make it easier to review? For the second commit it might also be nice to move file renamings (+ minor adjustments) into a commit separate from new tests and wholesale replacements of existing tests. |
Done. For whenever you have time. Thank you! |
| /// ``` | ||
| /// | ||
| /// FIXME(rcvalle): This allows a drop call on any trait object to call the drop function of any | ||
| /// other type. |
There was a problem hiding this comment.
Curious how C++ handles this. Is the assumption there just that there is no super class with virtual destructor shared across a ton of different classes?
There was a problem hiding this comment.
Clang handles virtual calls (including virtual destructors) using vcall checks (-fsanitize=cfi-vcall), which validate the vtable pointer rather than doing an icall function-pointer type check on the callee. During LTO, LLVM lays out vtables according to the class hierarchy and checks whether the vtable address point falls within the valid range (and bitset, when address points are interleaved due to multiple inheritance) for the static class type at the call site or one of its derived classes. Because C++ classes only share a vtable type id if they explicitly inherit from a common base class, a virtual destructor call on Base* only accepts vtables of Base and its subclasses (unless a codebase explicitly has a universal base class and deletes through it).
In Rust, every indirect call—including through vtables—is checked using icall (or KCFI) function-pointer type checks, and every trait object vtable contains a pointer to drop_in_place::<T>. When drop_in_place::<T> is defined, the compiler does not know all the traits T may be coerced to (including auto traits like dyn Send or traits in downstream crates), and drop_in_place::<T> is a single function shared across all vtables for T so both drop_in_place::<T> and virtual drop calls must be normalized to a common drop_in_place::<dyn Drop>.
|
I've now reviewed all the compiler side changes. I will leave reviewing the test suite refactor for later. Edit: Reviewed first test commit, LGTM. The second test commit is huge though. |
This comment has been minimized.
This comment has been minimized.
|
Would you mind splitting the second test commit out into a separate PR? And in any case this needs a rebase. @rustbot author |
rustbot
added
S-waiting-on-author
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
|
Requested reviewer is already assigned to this pull request. Please choose another assignee. |
Moves the DropGlue, virtual call, VTableShim, and closure-like transformations in transform_instance into the transform_drop_glue, transform_virtual_call, transform_vtable_shim, and transform_closure_like functions, and renames variables for consistency (e.g., invoke_ty to self_ty).
rcvalle
changed the title
|
@rustbot ready |
rustbot
added
S-waiting-on-review
Changes transform_instance to cover each InstanceKind (and ShimKind) explicitly, similarly to how encoding is done, so the intent is expressed clearly and it is known when an instance is handled (or not) and the side effects of it (also clearly), instead of relying on fallthrough behavior. This also makes adding a new InstanceKind (or ShimKind) result in a compile-time error until it is explicitly handled.
Moves the CFI and KCFI UI tests that are at the top level into the cfi and kcfi directories (removing the now redundant kcfi- prefix from their names), and renames the regression tests to <topic>-issue-<number> (removing their entries from issues.txt accordingly, and adding the missing regression test description to reveal-opaques-issue-114160.rs).
|
@bors r+ |
rust-bors
Bot
added
S-waiting-on-bors
…uwer Rollup of 8 pull requests Successful merges: - #163258 (Move `rustc_middle::ty::Const` to `rustc_type_ir` Part 2) - #164173 (don't drop `AnyOpaqueHasInferAsHidden` when merging with `OpaqueInStorageOrAnyOpaqueHasInferAsHidden`) - #160230 (CFI: Cover each InstanceKind explicitly) - #163713 (codegen Propagate reported errors from ABI checks) - #164205 (cg_llvm: Assorted cleanups to LLVM diagnostic handling) - #164114 (restore the old behavior for `derive(Default)` on fieldless types) - #164127 (core: Ignore explicit_tail_calls doc tests on PowerPC) - #164204 (explicitly mark next-solver ICE in test suite)
Rollup merge of #160230 - rcvalle:rust-cfi, r=bjorn3 CFI: Cover each InstanceKind explicitly Changes transform_instance to cover each InstanceKind (and ShimKind) explicitly, similarly to how encoding is done, so the intent is expressed clearly and it is known when an instance is handled (or not) and the side effects of it (also clearly), instead of relying on fallthrough behavior. This also makes adding a new InstanceKind (or ShimKind) result in a compile-time error until it is explicitly handled. No functional changes intended. r? @bjorn3
…uwer Rollup of 8 pull requests Successful merges: - rust-lang/rust#163258 (Move `rustc_middle::ty::Const` to `rustc_type_ir` Part 2) - rust-lang/rust#164173 (don't drop `AnyOpaqueHasInferAsHidden` when merging with `OpaqueInStorageOrAnyOpaqueHasInferAsHidden`) - rust-lang/rust#160230 (CFI: Cover each InstanceKind explicitly) - rust-lang/rust#163713 (codegen Propagate reported errors from ABI checks) - rust-lang/rust#164205 (cg_llvm: Assorted cleanups to LLVM diagnostic handling) - rust-lang/rust#164114 (restore the old behavior for `derive(Default)` on fieldless types) - rust-lang/rust#164127 (core: Ignore explicit_tail_calls doc tests on PowerPC) - rust-lang/rust#164204 (explicitly mark next-solver ICE in test suite)
|
Thank you very much for your time on this, @bjorn3! Much appreciated. |
…uwer Rollup of 8 pull requests Successful merges: - rust-lang/rust#163258 (Move `rustc_middle::ty::Const` to `rustc_type_ir` Part 2) - rust-lang/rust#164173 (don't drop `AnyOpaqueHasInferAsHidden` when merging with `OpaqueInStorageOrAnyOpaqueHasInferAsHidden`) - rust-lang/rust#160230 (CFI: Cover each InstanceKind explicitly) - rust-lang/rust#163713 (codegen Propagate reported errors from ABI checks) - rust-lang/rust#164205 (cg_llvm: Assorted cleanups to LLVM diagnostic handling) - rust-lang/rust#164114 (restore the old behavior for `derive(Default)` on fieldless types) - rust-lang/rust#164127 (core: Ignore explicit_tail_calls doc tests on PowerPC) - rust-lang/rust#164204 (explicitly mark next-solver ICE in test suite)
4 participants
View all comments
Changes transform_instance to cover each InstanceKind (and ShimKind) explicitly, similarly to how encoding is done, so the intent is expressed clearly and it is known when an instance is handled (or not) and the side effects of it (also clearly), instead of relying on fallthrough behavior. This also makes adding a new InstanceKind (or ShimKind) result in a compile-time error until it is explicitly handled. No functional changes intended.
r? @bjorn3