Skip to content

Conversation

djkoloski
Copy link
Contributor

Most combinations of LLVM sanitizers are legal-enough to enable simultaneously. This change will allow simultaneously enabling ASAN and shadow call stacks on supported platforms.

I used this python script to generate the mutually-exclusive sanitizer combinations:

#!/usr/bin/python3

import subprocess

flags = [
    ["-fsanitize=address"],
    ["-fsanitize=leak"],
    ["-fsanitize=memory"],
    ["-fsanitize=thread"],
    ["-fsanitize=hwaddress"],
    ["-fsanitize=cfi", "-flto", "-fvisibility=hidden"],
    ["-fsanitize=memtag", "--target=aarch64-linux-android", "-march=armv8a+memtag"],
    ["-fsanitize=shadow-call-stack"],
    ["-fsanitize=kcfi", "-flto", "-fvisibility=hidden"],
    ["-fsanitize=kernel-address"],
    ["-fsanitize=safe-stack"],
    ["-fsanitize=dataflow"],
]

for i in range(len(flags)):
    for j in range(i):
        command = ["clang++"] + flags[i] + flags[j] + ["-o", "main.o", "-c", "main.cpp"]
        completed = subprocess.run(command, stderr=subprocess.DEVNULL)
        if completed.returncode != 0:
            first = flags[i][0][11:].replace('-', '').upper()
            second = flags[j][0][11:].replace('-', '').upper()
            print(f"(SanitizerSet::{first}, SanitizerSet::{second}),")

@rustbot
Copy link
Collaborator

rustbot commented May 3, 2024

r? @estebank

rustbot has assigned @estebank.
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 May 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 3, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the PG-exploit-mitigations Project group: Exploit mitigations label May 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 3, 2024

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

@tmiasko
Copy link
Contributor

tmiasko commented May 10, 2024

Since Address Sanitizer includes the leak detection, combining it with Leak Sanitizer should omit the Leak Sanitizer runtime library:

https://github.com/rust-lang/llvm-project/blob/5399a24c66cb6164cf32280e7d300488c90d5765/clang/include/clang/Driver/SanitizerArgs.h#L92-L96

@ilovepi
Copy link
Contributor

ilovepi commented May 10, 2024

Since Address Sanitizer includes the leak detection, combining it with Leak Sanitizer should omit the Leak Sanitizer runtime library:

https://github.com/rust-lang/llvm-project/blob/5399a24c66cb6164cf32280e7d300488c90d5765/clang/include/clang/Driver/SanitizerArgs.h#L92-L96

That's a good point. I had assumed rustc already had similar logic, but I suppose if multiple sanitizers were disallowed, maybe it did not. This is some more evidence that on the LLVM side, we should probably keep that logic in a support library that frontends like clang and rustc can reuse without duplicating the same logic everywhere.

djkoloski added 3 commits May 15, 2024 15:40
Most combinations of LLVM sanitizers are legal-enough to enable
simultaneously. This change will allow simultaneously enabling ASAN and
shadow call stacks on supported platforms.
@djkoloski djkoloski force-pushed the relax_multiple_sanitizers branch from 8af9c9a to 5976494 Compare May 15, 2024 15:41
@djkoloski
Copy link
Contributor Author

r? @cuviper

@rustbot rustbot assigned cuviper and unassigned estebank May 15, 2024
@rcvalle
Copy link
Member

rcvalle commented May 16, 2024

LGTM (@cuviper FYI). Thank you for your time and for working on this, @djkoloski! Much appreciated.

@cuviper
Copy link
Member

cuviper commented May 20, 2024

Thanks!

@bors r=cuviper,rcvalle

@bors
Copy link
Collaborator

bors commented May 20, 2024

📌 Commit 1e1143c has been approved by cuviper,rcvalle

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 May 20, 2024
@bors
Copy link
Collaborator

bors commented May 21, 2024

⌛ Testing commit 1e1143c with merge 5065123...

@bors
Copy link
Collaborator

bors commented May 21, 2024

☀️ Test successful - checks-actions
Approved by: cuviper,rcvalle
Pushing 5065123 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 21, 2024
@bors bors merged commit 5065123 into rust-lang:master May 21, 2024
@rustbot rustbot added this to the 1.80.0 milestone May 21, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5065123): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.4% [1.0%, 1.7%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (primary -5.5%)

This 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-5.5% [-5.5%, -5.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -5.5% [-5.5%, -5.5%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 673.673s -> 670.767s (-0.43%)
Artifact size: 315.38 MiB -> 315.37 MiB (-0.00%)

bherrera pushed a commit to misttech/mist-os that referenced this pull request Aug 23, 2024
After rust-lang/rust#124676 was rolled, we can
enable SCS for both AArch64 and RISC-V, since it won't conflict with
other sanitizers in the compiler driver anymore.

Fixed: 42069386
Bug: 360955800
Change-Id: Ib30747204882317bb6b243c8a8ce760f09520a83
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/791504
Size-Review: Przemek Pietrzkiewicz <[email protected]>
Fuchsia-Auto-Submit: Paul Kirth <[email protected]>
Reviewed-by: Roland McGrath <[email protected]>
Commit-Queue: Paul Kirth <[email protected]>
bherrera pushed a commit to misttech/integration that referenced this pull request Aug 23, 2024
…n AArch64 and RISC-V

After rust-lang/rust#124676 was rolled, we can
enable SCS for both AArch64 and RISC-V, since it won't conflict with
other sanitizers in the compiler driver anymore.

Original-Fixed: 42069386
Original-Bug: 360955800
Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/791504
Size-Review: Przemek Pietrzkiewicz <[email protected]>
Original-Revision: 4eefc272d36835959f2e44be6e06a6fbb504e418
GitOrigin-RevId: 75ffba3794ae45c43c84d70f06549cf62eea483f
Change-Id: Ia2bc7342adff3cdc327fc37b8498ab7e9106e227
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.