Skip to content

Sanitizers target modificators #138736

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

azhogin
Copy link
Contributor

@azhogin azhogin commented Mar 20, 2025

Depends on bool flag fix: #138483.

Some sanitizers need to be target modifiers, and some do not. For now, we should mark all sanitizers as target modifiers except for these: AddressSanitizer, LeakSanitizer

For kCFI, the helper flag -Zsanitizer-cfi-normalize-integers should also be a target modifier.

Many test errors was with sanizer flags inconsistent with std deps. Tests are fixed with -C unsafe-allow-abi-mismatch.

@rustbot
Copy link
Collaborator

rustbot commented Mar 20, 2025

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
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 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 Mar 20, 2025
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 7526d38 to 0777169 Compare March 20, 2025 12:59
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 0777169 to f454b02 Compare March 24, 2025 08:53
@rustbot rustbot added the PG-exploit-mitigations Project group: Exploit mitigations label Mar 24, 2025
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from f454b02 to 509e0b3 Compare March 24, 2025 09:37
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 509e0b3 to 1f20a51 Compare March 24, 2025 11:33
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 1f20a51 to fc95e77 Compare March 24, 2025 13:14
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch 2 times, most recently from e025a55 to c54dcfc Compare March 24, 2025 13:40
@azhogin azhogin marked this pull request as ready for review March 24, 2025 14:46
@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2025

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
Copy link
Collaborator

bors commented Mar 26, 2025

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

@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 26, 2025

r? compiler

@Darksonn
Copy link
Contributor

Do we have any tests that check simultaneous use of multiple sanitizers? For example, if I enable both shadow-call-stack and kcfi at the same time, then both must also be enabled in deps.

@rcvalle rcvalle added the A-sanitizers Area: Sanitizers for correctness and code quality label Apr 10, 2025
@rcvalle
Copy link
Member

rcvalle commented Apr 10, 2025

@rustbot claim

@rustbot rustbot assigned rcvalle and unassigned oli-obk Apr 10, 2025
@@ -0,0 +1,7 @@
//@ no-prefer-dynamic
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should test file names be separated by hyphen ('-') instead?

@rcvalle
Copy link
Member

rcvalle commented Apr 15, 2025

Do we have any tests that check simultaneous use of multiple sanitizers? For example, if I enable both shadow-call-stack and kcfi at the same time, then both must also be enabled in deps.

Unfortunately, I don't think we do at this moment. (Others feel free to correct me if I'm wrong.) Something somewhat related that I also wanted us to have that we don't at the moment is build core and std and run their tests with some sanitizers enabled, such as CFI to make sure they currently work and no regressions/explicit CFI violations are introduced.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from c54dcfc to 3d27c96 Compare April 27, 2025 20:34
@azhogin
Copy link
Contributor Author

azhogin commented Apr 27, 2025

Renamed '_' to '-' in tests, added test with safestack + kcfi sanitizers. Btw, which target supports both kcfi and shadow-call-stack in the example?

About core/std build with sanitizers - I am not sure, how to integrate it to the current test system. Won't it be an overkill to build several versions of core/std in test procedure?

@rust-log-analyzer

This comment has been minimized.

…t modifiers with custom consistency check function
@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 3d27c96 to 280dfab Compare April 27, 2025 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sanitizers Area: Sanitizers for correctness and code quality 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants