LeakSanitizer, ThreadSanitizer, AddressSanitizer and MemorySanitizer support #38699
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
This is WIP revival of #31605. Right now, this only contains the Leak Sanitizer I'm bringing this back to life because one concern brought up in the original PR Comments
Concerns
TODO
|
| @@ -23,6 +23,9 @@ compiler_builtins = { path = "../libcompiler_builtins" } | |||
| std_unicode = { path = "../libstd_unicode" } | |||
| unwind = { path = "../libunwind" } | |||
|
|
|||
| [target.x86_64-unknown-linux-gnu.dependencies] | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
This is also a hack to place the rustc_lsan crate in the sysroot. (because std doesn't actually depend on rustc_lsan)
| @@ -0,0 +1,6 @@ | |||
| #![cfg_attr(not(stage0), feature(sanitizer_runtime))] | |||
There was a problem hiding this comment.
I suppose this crate should also contain the -lpthread -lrt -lm linker flags that runtimes supossedly depend on. But I'm not sure what runtime depends on what library.
There was a problem hiding this comment.
Eh that's ok b/c they're all pulled in through libstd anyway
|
☔ The latest upstream changes (presumably #38697) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@japaric I'm excited to hear that xargo can compile std with custom flags. However, I feel like there is an RFC wanting to be written here. But I'm not quite sure what it is. =) At minimum, surely a feature gate of some kind would be required. |
|
Neat! How come we need to specially recognize the asan crate? Does it need to go in a special place on the linker command line? I'm not really sure what the best way to expose this will be long-term. For now, though, having it be optional and behind a feature gate seems ok. |
(a) It needs to be linked using |
|
I think the ordering is guaranteed via |
|
@alexcrichton That is, indeed, the case. It doesn't work without |
|
Travis failure seems legit to me: "llvm-config not found". |
|
OK. Added ThreadSanitizer support and added an unstable Using LeakSanitizer is as simple as (*) Actually, first you have patch --- a/src/libstd/Cargo.toml
+++ b/src/libstd/Cargo.toml
@@ -7,7 +7,6 @@ build = "build.rs"
[lib]
name = "std"
path = "lib.rs"
-crate-type = ["dylib", "rlib"]
[dependencies]
alloc = { path = "../liballoc" }Some comments: LeakSanitizer only requires linking a runtime so, IMO, it makes sense to ThreadSanitizer requires recompiling the rust-lang/compiler-rt needs to be patched to make tsan work with PIE |
japaric
changed the title
src/librustc_llvm/lib.rs
Outdated
| @@ -432,3 +432,4 @@ impl Drop for OperandBundleDef { | |||
| mod llvmdeps { | |||
| include! { env!("CFG_LLVM_LINKAGE_FILE") } | |||
| } | |||
| #[link(name = "ffi")] extern {} | |||
There was a problem hiding this comment.
TODO remove. (Sometimes I forget to not commit this -- workaround for #34486)
|
Pushed AddressSanitizer support and added an example to the PR description. Interestingly, an empty |
japaric
changed the title
|
travis says: not sure what's up w/ that |
|
Quoting myself
Travis is testing on Ubuntu. We should build MemorySanitizer is now working (example in the PR description) but requires the latest compiler-rt release. We could backport some stuff to make our fork work (but we would have to first figure out what needs to be backported) or we could simply upgrade our fork to something more recent (easier). |
japaric
changed the title
|
So, @alexcrichton -- I feel more-or-less about experimenting in this direction without an RFC, but it seems like we should have some docs. Maybe a tracking issue? I feel like I would not want this interface to stabilize without an RFC. Not sure what team has jurisdiction here, probably @rust-lang/compiler or @rust-lang/tools. |
|
Yeah I'm fine experimenting here as well. I want to make sure that everything is thoroughly feature gated, but beyond that it seems worthwhile to enable supporting this at all |
|
I wonder though if we can experiment here with less of this support in-tree. @japaric do you have an idea of how we might minimize basically the size of this PR? Ideally we wouldn't maintain all of this in tree because development would be slowed down, and hopefully one day these crates can all be on crates.io anyway. I'm slightly hesitant here as this would be one of the first "linux only" features we support in-tree, so if we can keep it external that would be nice. |
src/librustc_trans/back/link.rs
Outdated
| // We must link the sanitizer runtime using -Wl,--whole-archive but since | ||
| // it's packed in a .rlib, it contains stuff that are not objects that will | ||
| // make the linker error. So we must remove those bits from the .rlib before | ||
| // linking it. |
There was a problem hiding this comment.
Why does it need --whole-archive?
There was a problem hiding this comment.
Because of the dependency graph (the sanitizer runtime gets injected as a direct dependency of the "top" crate) the runtime rlib ends up being one of the first linker arguments. Because linkers are lazy, if we don't use whole-archive, the linker will end up dropping most / all of the symbols in it (because the preceding linker argument didn't directly need the symbols). whole-archive prevents that; it forces the linker to keep all the symbols around as it keeps looking at the next linker arguments.
|
@alexcrichton Initial testing with rustc_lsan shows that the sanitizer runtimes can live out of tree. I'll update the PR with that in mind. |
|
Great work, @japaric! Some thoughts on integration into the compiler:
|
|
@japaric It is great to see more work on sanitizer support! Regarding the question how much of this should be in-tree or not, I think the |
|
@bors r=alexcrichton |
|
📌 Commit e180dd5 has been approved by |
LeakSanitizer, ThreadSanitizer, AddressSanitizer and MemorySanitizer support
```
$ cargo new --bin leak && cd $_
$ edit Cargo.toml && tail -n3 $_
```
``` toml
[profile.dev]
opt-level = 1
```
```
$ edit src/main.rs && cat $_
```
``` rust
use std::mem;
fn main() {
let xs = vec![0, 1, 2, 3];
mem::forget(xs);
}
```
```
$ RUSTFLAGS="-Z sanitizer=leak" cargo run --target x86_64-unknown-linux-gnu; echo $?
Finished dev [optimized + debuginfo] target(s) in 0.0 secs
Running `target/debug/leak`
=================================================================
==10848==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 16 byte(s) in 1 object(s) allocated from:
#0 0x557c3488db1f in __interceptor_malloc /shared/rust/checkouts/lsan/src/compiler-rt/lib/lsan/lsan_interceptors.cc:55
#1 0x557c34888aaa in alloc::heap::exchange_malloc::h68f3f8b376a0da42 /shared/rust/checkouts/lsan/src/liballoc/heap.rs:138
rust-lang#2 0x557c34888afc in leak::main::hc56ab767de6d653a $PWD/src/main.rs:4
rust-lang#3 0x557c348c0806 in __rust_maybe_catch_panic ($PWD/target/debug/leak+0x3d806)
SUMMARY: LeakSanitizer: 16 byte(s) leaked in 1 allocation(s).
23
```
```
$ cargo new --bin racy && cd $_
$ edit src/main.rs && cat $_
```
``` rust
use std::thread;
static mut ANSWER: i32 = 0;
fn main() {
let t1 = thread::spawn(|| unsafe { ANSWER = 42 });
unsafe {
ANSWER = 24;
}
t1.join().ok();
}
```
```
$ RUSTFLAGS="-Z sanitizer=thread" cargo run --target x86_64-unknown-linux-gnu; echo $?
==================
WARNING: ThreadSanitizer: data race (pid=12019)
Write of size 4 at 0x562105989bb4 by thread T1:
#0 racy::main::_$u7b$$u7b$closure$u7d$$u7d$::hbe13ea9e8ac73f7e $PWD/src/main.rs:6 (racy+0x000000010e3f)
#1 _$LT$std..panic..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..FnOnce$LT$$LP$$RP$$GT$$GT$::call_once::h2e466a92accacc78 /shared/rust/checkouts/lsan/src/libstd/panic.rs:296 (racy+0x000000010cc5)
rust-lang#2 std::panicking::try::do_call::h7f4d2b38069e4042 /shared/rust/checkouts/lsan/src/libstd/panicking.rs:460 (racy+0x00000000c8f2)
rust-lang#3 __rust_maybe_catch_panic <null> (racy+0x0000000b4e56)
rust-lang#4 std::panic::catch_unwind::h31ca45621ad66d5a /shared/rust/checkouts/lsan/src/libstd/panic.rs:361 (racy+0x00000000b517)
rust-lang#5 std::thread::Builder::spawn::_$u7b$$u7b$closure$u7d$$u7d$::hccfc37175dea0b01 /shared/rust/checkouts/lsan/src/libstd/thread/mod.rs:357 (racy+0x00000000c226)
rust-lang#6 _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::hd880bbf91561e033 /shared/rust/checkouts/lsan/src/liballoc/boxed.rs:605 (racy+0x00000000f27e)
rust-lang#7 std::sys::imp::thread::Thread::new::thread_start::hebdfc4b3d17afc85 <null> (racy+0x0000000abd40)
Previous write of size 4 at 0x562105989bb4 by main thread:
#0 racy::main::h23e6e5ca46d085c3 $PWD/src/main.rs:8 (racy+0x000000010d7c)
#1 __rust_maybe_catch_panic <null> (racy+0x0000000b4e56)
rust-lang#2 __libc_start_main <null> (libc.so.6+0x000000020290)
Location is global 'racy::ANSWER::h543d2b139f819b19' of size 4 at 0x562105989bb4 (racy+0x0000002f8bb4)
Thread T1 (tid=12028, running) created by main thread at:
#0 pthread_create /shared/rust/checkouts/lsan/src/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:902 (racy+0x00000001aedb)
#1 std::sys::imp::thread::Thread::new::hce44187bf4a36222 <null> (racy+0x0000000ab9ae)
rust-lang#2 std::thread::spawn::he382608373eb667e /shared/rust/checkouts/lsan/src/libstd/thread/mod.rs:412 (racy+0x00000000b5aa)
rust-lang#3 racy::main::h23e6e5ca46d085c3 $PWD/src/main.rs:6 (racy+0x000000010d5c)
rust-lang#4 __rust_maybe_catch_panic <null> (racy+0x0000000b4e56)
rust-lang#5 __libc_start_main <null> (libc.so.6+0x000000020290)
SUMMARY: ThreadSanitizer: data race $PWD/src/main.rs:6 in racy::main::_$u7b$$u7b$closure$u7d$$u7d$::hbe13ea9e8ac73f7e
==================
ThreadSanitizer: reported 1 warnings
66
```
```
$ cargo new --bin oob && cd $_
$ edit src/main.rs && cat $_
```
``` rust
fn main() {
let xs = [0, 1, 2, 3];
let y = unsafe { *xs.as_ptr().offset(4) };
}
```
```
$ RUSTFLAGS="-Z sanitizer=address" cargo run --target x86_64-unknown-linux-gnu; echo $?
=================================================================
==13328==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff29f3ecd0 at pc 0x55802dc6bf7e bp 0x7fff29f3ec90 sp 0x7fff29f3ec88
READ of size 4 at 0x7fff29f3ecd0 thread T0
#0 0x55802dc6bf7d in oob::main::h0adc7b67e5feb2e7 $PWD/src/main.rs:3
#1 0x55802dd60426 in __rust_maybe_catch_panic ($PWD/target/debug/oob+0xfe426)
rust-lang#2 0x55802dd58dd9 in std::rt::lang_start::hb2951fc8a59d62a7 ($PWD/target/debug/oob+0xf6dd9)
rust-lang#3 0x55802dc6c002 in main ($PWD/target/debug/oob+0xa002)
rust-lang#4 0x7fad8c3b3290 in __libc_start_main (/usr/lib/libc.so.6+0x20290)
rust-lang#5 0x55802dc6b719 in _start ($PWD/target/debug/oob+0x9719)
Address 0x7fff29f3ecd0 is located in stack of thread T0 at offset 48 in frame
#0 0x55802dc6bd5f in oob::main::h0adc7b67e5feb2e7 $PWD/src/main.rs:1
This frame has 1 object(s):
[32, 48) 'xs' <== Memory access at offset 48 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
(longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow $PWD/src/main.rs:3 in oob::main::h0adc7b67e5feb2e7
Shadow bytes around the buggy address:
0x1000653dfd40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1000653dfd50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1000653dfd60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1000653dfd70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1000653dfd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x1000653dfd90: 00 00 00 00 f1 f1 f1 f1 00 00[f3]f3 00 00 00 00
0x1000653dfda0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1000653dfdb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1000653dfdc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1000653dfdd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1000653dfde0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Heap right redzone: fb
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack partial redzone: f4
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==13328==ABORTING
1
```
```
$ cargo new --bin uninit && cd $_
$ edit src/main.rs && cat $_
```
``` rust
use std::mem;
fn main() {
let xs: [u8; 4] = unsafe { mem::uninitialized() };
let y = xs[0] + xs[1];
}
```
```
$ RUSTFLAGS="-Z sanitizer=memory" cargo run; echo $?
==30198==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x563f4b6867da in uninit::main::hc2731cd4f2ed48f8 $PWD/src/main.rs:5
#1 0x563f4b7033b6 in __rust_maybe_catch_panic ($PWD/target/debug/uninit+0x873b6)
rust-lang#2 0x563f4b6fbd69 in std::rt::lang_start::hb2951fc8a59d62a7 ($PWD/target/debug/uninit+0x7fd69)
rust-lang#3 0x563f4b6868a9 in main ($PWD/target/debug/uninit+0xa8a9)
rust-lang#4 0x7fe844354290 in __libc_start_main (/usr/lib/libc.so.6+0x20290)
rust-lang#5 0x563f4b6864f9 in _start ($PWD/target/debug/uninit+0xa4f9)
SUMMARY: MemorySanitizer: use-of-uninitialized-value $PWD/src/main.rs:5 in uninit::main::hc2731cd4f2ed48f8
Exiting
77
```
|
Should this be marked with relnotes tag? |
|
@alexcrichton Done. #39699 |
|
Thanks, @japaric , what a great feature! |
|
@japaric @alexcrichton This PR integrates with rustbuild in a really weird way. It adds some always-enabled 'optional' std features, lsan/msan/asan, and then omits actually producing any code for them if the LLVM_CONFIG environment variable is absent; the LLVM_CONFIG variable is only added if --enable-sanitizers is passed. Shouldn't there be more sanity in this? Actually enabling the feature only if the config flag is on? |
|
@whitequark sounds reasonable to me, yeah. We may also be able to remove the features now as they were from a previous iteration as well. @japaric what do you think? |
|
Yeah, seems fine to remove the Cargo features. I'll prepare a PR. |
Examples
LeakSanitizer
ThreadSanitizer
AddressSanitizer
MemorySanitizer