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.

Sign up for GitHub

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

Jump to bottom

Add MemTagSanitizer Support #91675

Merged
merged 1 commit into from
Feb 19, 2022
Merged

Add MemTagSanitizer Support #91675

merged 1 commit into from
Feb 19, 2022

Conversation

ivanloz
Copy link
Contributor

@ivanloz ivanloz commented Dec 8, 2021

Add support for the LLVM MemTagSanitizer.

On hardware which supports it (see caveats below), the MemTagSanitizer can catch bugs similar to AddressSanitizer and HardwareAddressSanitizer, but with lower overhead.

On a tag mismatch, a SIGSEGV is signaled with code SEGV_MTESERR / SEGV_MTEAERR.

Usage

-Zsanitizer=memtag -C target-feature="+mte"

Comments/Caveats

  • MemTagSanitizer is only supported on AArch64 targets with hardware support
  • Requires -C target-feature="+mte"
  • LLVM MemTagSanitizer currently only performs stack tagging.

TODO

  • Tests
  • Example

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 8, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 9, 2021
@ivanloz ivanloz marked this pull request as draft December 9, 2021 14:45
@ivanloz ivanloz force-pushed the memtagsan branch 2 times, most recently from 77bd3d1 to 9b15ee4 Compare December 9, 2021 21:02
@ivanloz
Copy link
Contributor Author

ivanloz commented Dec 9, 2021

Added a basic test which checks that the attribute is being emitted as expected.

@ivanloz ivanloz marked this pull request as ready for review December 9, 2021 21:05
davidtwco
davidtwco approved these changes Dec 13, 2021
View reviewed changes
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.

This looks reasonable to me but it's not an area that I'm familiar with.

@davidtwco
Copy link
Member

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned davidtwco Dec 13, 2021
@tmiasko
Copy link
Contributor

tmiasko commented Dec 13, 2021

MemTagSanitizer requires hardware support and the mte target feature.

Could we diagnose the attempt to use the sanitizer without mte target feature? This seems like the first issue one would generally encounter when trying to use the sanitizer.

FWIW the implementation looks good to me.

@ivanloz
Copy link
Contributor Author

ivanloz commented Dec 14, 2021

Tested passing the sanitizer without the mte target feature, and LLVM throws out a pretty difficult to understand error.

So I've gone ahead and added a check such that rustc errors out if the sanitizer is used without the target feature and emits a useful error message indicating the mte target feature is required.

@nagisa nagisa 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 Dec 27, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@ivanloz can you please address the comments from @nagisa?

@ivanloz
Copy link
Contributor Author

ivanloz commented Jan 24, 2022

Apologies and thanks for your patience, this has been on my radar. I expect to have time to address the comments this week.

@nagisa
Copy link
Member

nagisa commented Feb 11, 2022

r=me with or without the nit fixed.

@bors
Copy link
Contributor

bors commented Feb 13, 2022

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

@ivanloz
Copy link
Contributor Author

ivanloz commented Feb 15, 2022

Merge conflicts should be resolved now.

@nagisa
Copy link
Member

nagisa commented Feb 15, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 15, 2022

📌 Commit da23682322ec6183f6968068ac2a99565c60abd8 has been approved by nagisa

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 15, 2022
@bors
Copy link
Contributor

bors commented Feb 16, 2022

⌛ Testing commit da23682322ec6183f6968068ac2a99565c60abd8 with merge a08fcc4c75532ca5b4a508d561c9fe753e31333d...

@bors
Copy link
Contributor

bors commented Feb 16, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 16, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ivanloz
MemTagSanitizer Support
568aeda 
Adds support for the LLVM MemTagSanitizer.
@ivanloz
Copy link
Contributor Author

ivanloz commented Feb 16, 2022

I removed the quotes from the test's 'target-features' arg -- seems like that may have been causing the testing infrastructure to fail.

@nagisa
Copy link
Member

nagisa commented Feb 16, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 16, 2022

📌 Commit 568aeda has been approved by nagisa

@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 Feb 16, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 18, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2022
@bors
Auto merge of rust-lang#94134 - matthiaskrgr:rollup-b132kjz, r=matthi…
1882597 
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#89892 (Suggest `impl Trait` return type when incorrectly using a generic return type)
 - rust-lang#91675 (Add MemTagSanitizer Support)
 - rust-lang#92806 (Add more information to `impl Trait` error)
 - rust-lang#93497 (Pass `--test` flag through rustdoc to rustc so `#[test]` functions can be scraped)
 - rust-lang#93814 (mips64-openwrt-linux-musl: correct soft-foat)
 - rust-lang#93847 (kmc-solid: Use the filesystem thread-safety wrapper)
 - rust-lang#93877 (asm: Allow the use of r8-r14 as clobbers on Thumb1)
 - rust-lang#93892 (Only mark projection as ambiguous if GAT substs are constrained)
 - rust-lang#93915 (Implement --check-cfg option (RFC 3013), take 2)
 - rust-lang#93953 (Add the `known-bug` test directive, use it, and do some cleanup)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0bb72a2 into rust-lang:master Feb 19, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagisa nagisa nagisa left review comments

@tmiasko tmiasko tmiasko left review comments

@davidtwco davidtwco davidtwco approved these changes

Assignees

@nagisa nagisa

Labels
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
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
10 participants
@ivanloz @rust-highfive @rust-log-analyzer @davidtwco @tmiasko @JohnCSimon @nagisa @bors @apiraino @rustbot