Skip to content

Add support for -Zsanitizer-cfi-minimal-runtime - #162493

Open
jakos-sec wants to merge 11 commits into
rust-lang:mainfrom
jakos-sec:ubsan-runtime-minimal
Open

Add support for -Zsanitizer-cfi-minimal-runtime#162493
jakos-sec wants to merge 11 commits into
rust-lang:mainfrom
jakos-sec:ubsan-runtime-minimal

Conversation

@jakos-sec

Copy link
Copy Markdown
Contributor

For production use, we should only link in the ubsan_minimal runtime,
instead of the complete ubsan runtime. This adds support for both
cfi-recover and cfi-diag to use the minimal runtime when
-Zsanitizer-cfi-minimal-runtime is specified.

This also includes tests, to ensure the flag can only be used if either
cfi-recover or cfi-diag is enabled, it doesn't disrupt the original
behavior, and links in the correct runtime when specified.

cc @1c3t3a

?r rcvalle

Co-Authored-By: Bastian Kersting <bkersting@google.com>
@rustbot

rustbot commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in tests/codegen-llvm/sanitizer

cc @rcvalle

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

Some changes occurred in tests/ui/sanitizer

cc @rcvalle

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-run-make Area: port run-make Makefiles to rmake.rs PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 8, 2026
@rustbot

rustbot commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

r? @folkertdev

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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 76 candidates
  • Random selection from 20 candidates

@folkertdev

Copy link
Copy Markdown
Contributor

r? rcvalle

@rustbot rustbot assigned rcvalle and unassigned folkertdev Sep 8, 2026
Comment thread compiler/rustc_codegen_llvm/src/builder.rs
@rcvalle

rcvalle commented Sep 8, 2026

Copy link
Copy Markdown
Member

@jakos-sec

Copy link
Copy Markdown
Contributor Author

Would you mind adding tests it similarly to https://github.com/rust-lang/rust/blob/main/tests/ui/sanitizer/cfi/generalize-pointers-requires-cfi.rs and https://github.com/rust-lang/rust/blob/main/tests/ui/sanitizer/cfi/normalize-integers-requires-cfi.rs (i.e., requires CFI, not just CFI diagnostics or CFI recovery)?

I'm not entirely sure what the best behavior is here. -Zsanitizer-cfi-minimal-runtime is strictly dependent on -Zsanitizer-cfi-recover / -Zsanitizer-cfi-diag (and both of those are dependent on -Zsanitizer=cfi.

Now that we have tests that cfi-diag and cfi-recover require cfi, this should already be enough. Otherwise we would need to add a separate error message for all the different cases (you need a specific error message for -Zsanitizer-cfi-minimal-runtime if used without cfi, without cfi-recover/diag or used without both).

@jakos-sec
jakos-sec force-pushed the ubsan-runtime-minimal branch from b2af13f to 5ee5ebd Compare September 10, 2026 14:43
@rust-log-analyzer

This comment has been minimized.

@jakos-sec

Copy link
Copy Markdown
Contributor Author

The job test-aarch64-gnu-llvm-21-1 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Failing tests I need to look at / fix before another round of reviews.

@rcvalle

rcvalle commented Sep 10, 2026

Copy link
Copy Markdown
Member

Would you mind adding tests it similarly to https://github.com/rust-lang/rust/blob/main/tests/ui/sanitizer/cfi/generalize-pointers-requires-cfi.rs and https://github.com/rust-lang/rust/blob/main/tests/ui/sanitizer/cfi/normalize-integers-requires-cfi.rs (i.e., requires CFI, not just CFI diagnostics or CFI recovery)?

Would you mind adding tests it similarly to https://github.com/rust-lang/rust/blob/main/tests/ui/sanitizer/cfi/generalize-pointers-requires-cfi.rs and https://github.com/rust-lang/rust/blob/main/tests/ui/sanitizer/cfi/normalize-integers-requires-cfi.rs (i.e., requires CFI, not just CFI diagnostics or CFI recovery)?

I'm not entirely sure what the best behavior is here. -Zsanitizer-cfi-minimal-runtime is strictly dependent on -Zsanitizer-cfi-recover / -Zsanitizer-cfi-diag (and both of those are dependent on -Zsanitizer=cfi.

Now that we have tests that cfi-diag and cfi-recover require cfi, this should already be enough. Otherwise we would need to add a separate error message for all the different cases (you need a specific error message for -Zsanitizer-cfi-minimal-runtime if used without cfi, without cfi-recover/diag or used without both).

Yes, I think this is how the Rust compiler does it and historically this is how we've been doing it. This also aligns with the philosophy of having clear and helpful error messages. In this case, with only one incorrect run, the user would be able to see both requirements without requiring one additional run.

jakos-sec and others added 6 commits September 11, 2026 07:42
For production use, we should only link in the ubsan_minimal runtime,
instead of the complete ubsan runtime. This adds support for both
cfi-recover and cfi-diag to use the minimal runtime when
`-Zsanitizer-cfi-minimal-runtime` is specified.

This also includes tests, to ensure the flag can only be used if either
cfi-recover or cfi-diag is enabled, it doesn't disrupt the original
behavior, and links in the correct runtime when specified.

Co-Authored-By: Bastian Kersting <bkersting@google.com>
Co-Authored-By: Bastian Kersting <bkersting@google.com>
@jakos-sec
jakos-sec force-pushed the ubsan-runtime-minimal branch from 5ee5ebd to b47e873 Compare September 11, 2026 09:02
@jakos-sec

Copy link
Copy Markdown
Contributor Author

Would you mind adding tests it similarly to https://github.com/rust-lang/rust/blob/main/tests/ui/sanitizer/cfi/generalize-pointers-requires-cfi.rs and https://github.com/rust-lang/rust/blob/main/tests/ui/sanitizer/cfi/normalize-integers-requires-cfi.rs (i.e., requires CFI, not just CFI diagnostics or CFI recovery)?

Would you mind adding tests it similarly to https://github.com/rust-lang/rust/blob/main/tests/ui/sanitizer/cfi/generalize-pointers-requires-cfi.rs and https://github.com/rust-lang/rust/blob/main/tests/ui/sanitizer/cfi/normalize-integers-requires-cfi.rs (i.e., requires CFI, not just CFI diagnostics or CFI recovery)?

I'm not entirely sure what the best behavior is here. -Zsanitizer-cfi-minimal-runtime is strictly dependent on -Zsanitizer-cfi-recover / -Zsanitizer-cfi-diag (and both of those are dependent on -Zsanitizer=cfi.
Now that we have tests that cfi-diag and cfi-recover require cfi, this should already be enough. Otherwise we would need to add a separate error message for all the different cases (you need a specific error message for -Zsanitizer-cfi-minimal-runtime if used without cfi, without cfi-recover/diag or used without both).

Yes, I think this is how the Rust compiler does it and historically this is how we've been doing it. This also aligns with the philosophy of having clear and helpful error messages. In this case, with only one incorrect run, the user would be able to see both requirements without requiring one additional run.

Makes sense, should be added now.

The job test-aarch64-gnu-llvm-21-1 failed! Check out the build log: (web) (plain enhanced) (plain)
Click to see the possible cause of the failure (guessed by this bot)

Failing tests I need to look at / fix before another round of reviews.

These should also be addressed now.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-bors

rust-bors Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #162631) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-run-make Area: port run-make Makefiles to rmake.rs PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

6 participants