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.

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

sanitizers: Create the rustc_sanitizers crate #123620

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

rcvalle
Copy link
Member

@rcvalle rcvalle commented Apr 8, 2024

Create the rustc_sanitizers crate and move the source code for the CFI and KCFI sanitizers to it. The tracking issue for reviewing and moving sanitizers into a compiler crate is #123619. This is part of our work to organize and stabilize support for the sanitizers. (See our roadmap at https://hackmd.io/@rcvalle/S1Ou9K6H6.)

@rustbot
Copy link
Collaborator

rustbot commented Apr 8, 2024

r? @fmease

rustbot has assigned @fmease.
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

@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations 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 Apr 8, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 8, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rcvalle
Copy link
Member Author

rcvalle commented Apr 8, 2024

r? @davidtwco

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

LGTM, lots of small comments, but this is otherwise a clear improvement

compiler/rustc_sanitizers/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_sanitizers/src/kcfi/typeid/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_sanitizers/src/kcfi/mod.rs Outdated Show resolved Hide resolved
/// This crate contains the source code for providing support for the sanitizers to the Rust
/// compiler.
pub mod cfi;
pub mod kcfi;
Copy link
Member

Choose a reason for hiding this comment

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

nit: we could probably simplify the file structure a little bit here, it's mostly an aesthetic concern so only if you agree:

rustc_sanitizers
|- lib.rs
|- cfi.rs (containing what is in cfi/typeid/mod.rs)
|- kcfi.rs (containing what is in kcfi/typeid/mod.rs)
 \- typeid/* (private mod, containing what is in cfi/typeid/itanium_cxx_abi now)

It's also possible that this doesn't make sense anticipating some of the other things you want to move to this crate that exists elsewhere at the moment, but if we're only anticipating things that we might do by having this structure (e.g. manglings other than itanium), then we can change to this structure when we're adding that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I thought about it too. The reason I ended up doing it this way was because I'm anticipating two things:

  1. Move as much as possible of CFI and KCFI related source code to cfi/mod.rs and kcfi/mod.rs (e.g., source code at declare.rs and builder.rs as cfi::emit_type_test and kcfi::emit_kcfi_bundle).
  2. Add possibly at least an additional encoding for Microsoft XFG and cross-language Microsoft XFG (but we might want to discuss if the rustc_sanitizers crate is the right place for it), or a more coarse-grained encoding for cross-language CFI compatibility with other languages.

What do you think?

compiler/rustc_sanitizers/src/cfi/typeid/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_sanitizers/src/cfi/mod.rs Outdated Show resolved Hide resolved
@rustbot rustbot added S-waiting-on-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 Apr 8, 2024
@rcvalle rcvalle force-pushed the rust-create-rustc-sanitizers branch from cc37b8e to 5e9d4df Compare April 8, 2024 18:51
Create the rustc_sanitizers crate and move the source code for the CFI
and KCFI sanitizers to it.

Co-authored-by: David Wood <agile.lion3441@fuligin.ink>
@rcvalle rcvalle force-pushed the rust-create-rustc-sanitizers branch from 5e9d4df to 1f0f2c4 Compare April 8, 2024 19:06
@rcvalle
Copy link
Member Author

rcvalle commented Apr 8, 2024

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 8, 2024
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 8, 2024

📌 Commit 1f0f2c4 has been approved by davidtwco

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 Apr 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#122768 (Use the more informative generic type inference failure error on method calls on raw pointers)
 - rust-lang#123620 (sanitizers: Create the rustc_sanitizers crate)
 - rust-lang#123624 ([rustdoc] [GUI tests] Make theme switching closer to reality)
 - rust-lang#123636 (Update books)
 - rust-lang#123647 (rustdoc: slightly clean up the synthesis of blanket impls)
 - rust-lang#123648 (Avoid ICEing without the pattern_types feature gate)
 - rust-lang#123649 (KCFI: Use legal charset in shim encoding)
 - rust-lang#123652 (Fix UI tests with dist-vendored dependencies)
 - rust-lang#123655 (Remove unimplemented!() from BinOp::ty() function)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b809c42 into rust-lang:master Apr 9, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2024
Rollup merge of rust-lang#123620 - rcvalle:rust-create-rustc-sanitizers, r=davidtwco

sanitizers: Create the rustc_sanitizers crate

Create the `rustc_sanitizers` crate and move the source code for the CFI and KCFI sanitizers to it. The tracking issue for reviewing and moving sanitizers into a compiler crate is rust-lang#123619. This is part of our work to organize and stabilize support for the sanitizers. (See our roadmap at https://hackmd.io/`@rcvalle/S1Ou9K6H6.)`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants