Tracking Issue for stabilizing the sanitizers (e.g., AddressSanitizer, LeakSanitizer, MemorySanitizer, ThreadSanitizer)

rcvalle commented 7 months ago

This is a tracking issue for stabilizing the sanitizers (e.g., AddressSanitizer, LeakSanitizer, MemorySanitizer, ThreadSanitizer). The original tracking issue for the sanitizers support for Rust is #39699. This is part of our work to organize and stabilize support for the sanitizers. (See our roadmap at https://hackmd.io/@rcvalle/S1Ou9K6H6.)

Steps

[ ] Add support for separating sanitizers support in stable and unstable for supported targets.
[ ] Add core sanitizers to the stable sanitizers for Tier 1 targets.
[ ] TBD.

Unresolved Questions

Is there any concern or anything that the community would like to see implemented before their stabilization?

Implementation history

123617

rcvalle commented 7 months ago

@davidtwco and I would like to propose initially stabilizing AddressSanitizer and LeakSanitizer for the Tier 1 targets that support them. After stabilizing these sanitizers, the supported sanitizers will look like this--we'll keep this supported sanitizers table updated as we stabilize the sanitizers:

Target	Supported Sanitizers (Stable)	Supported Sanitizers (Unstable)
aarch64-apple-darwin		address, cfi, thread
aarch64-apple-ios		address, thread
aarch64-apple-ios-macabi		address, leak, thread
aarch64-apple-ios-sim		address, thread
aarch64-apple-visionos		address, thread
aarch64-apple-visionos-sim		address, thread
aarch64-fuchsia		address, cfi, shadow-call-stack
aarch64-linux-android		address, cfi, hwaddress, memtag, shadow-call-stack
aarch64-unknown-freebsd		address, cfi, memory, thread
aarch64-unknown-fuchsia		address, cfi, shadow-call-stack
aarch64-unknown-illumos		address, cfi
aarch64-unknown-linux-gnu	address, leak	cfi, hwaddress, kcfi, memory, memtag, thread
aarch64-unknown-linux-musl		address, cfi, leak, memory, thread
aarch64-unknown-linux-ohos		address, cfi, hwaddress, leak, memory, memtag, thread
aarch64-unknown-none		kcfi, kernel-address
arm-linux-androideabi		address
arm64e-apple-darwin		address, cfi, thread
arm64e-apple-ios		address, thread
armv7-linux-androideabi		address
i586-pc-windows-msvc	address
i586-unknown-linux-gnu	address
i686-linux-android		address
i686-pc-windows-msvc	address
i686-unknown-linux-gnu	address
riscv64-linux-android		address
riscv64gc-unknown-fuchsia		shadow-call-stack
riscv64gc-unknown-none-elf		kernel-address
riscv64imac-unknown-none-elf		kernel-address
s390x-unknown-linux-gnu		address, leak, memory, thread
s390x-unknown-linux-musl		address, leak, memory, thread
thumbv7neon-linux-androideabi		address
x86_64-apple-darwin	address, leak	cfi, thread
x86_64-apple-ios		address, thread
x86_64-apple-ios-macabi		address, leak, thread
x86_64-fuchsia		address, cfi, leak
x86_64-linux-android		address
x86_64-pc-solaris		address, cfi, thread
x86_64-pc-windows-msvc	address
x86_64-unknown-freebsd		address, cfi, memory, thread
x86_64-unknown-fuchsia		address, cfi, leak
x86_64-unknown-illumos		address, cfi, thread
x86_64-unknown-linux-gnu	address, leak	cfi, dataflow, kcfi, memory, safestack, thread
x86_64-unknown-linux-musl		address, cfi, leak, memory, thread
x86_64-unknown-linux-ohos		address, cfi, leak, memory, thread
x86_64-unknown-netbsd		address, cfi, leak, memory, thread
x86_64-unknown-none		kcfi, kernel-address
x86_64h-apple-darwin		address, cfi, leak, thread

Jules-Bertholet commented 7 months ago

@rustbot label A-sanitizers

briansmith commented 5 months ago

Is there any concern or anything that the community would like to see implemented before their stabilization?

cfg(sanitizer = "known-but-unstable-sanitizer") should trigger a warning that can be disabled on non-Nightly Rust, instead of triggering the "use of an unstable feature". Otherwise crates that want to support stable Rust need to add feature flags or equivalent to guard their #[cfg(sanizier)] usage whenever they do something to support an unstable sanitizer.
cfg(sanitizer = "unknown-sanitizer") should trigger a warning that can be disabled on non-Nightly Rust, for the same reason. Perhaps it should be a separate lint.

briansmith commented 5 months ago

Is there any concern or anything that the community would like to see implemented before their stabilization?

We are lacking an official way to __msan_unpoison, which is necessary for many things that skip libc and directly invoke syscalls, e.g. via inline assembly or libc::syscall or equivalent. Similarly it is needed for inline assembly because MSAN (at least with Clang's inline assembly) quite reasonably doesn't track writes via inline assembly. Currently I just declare __msan_unpoison as extern "C" and call it. Ideally there would be an official API for this.
When sanitizers are active, what should we assume about how functions like MaybeUninit::{assume_init, assume_init_drop, assume_init_mut, assume_init_ref, slice::{from_raw_parts,from_raw_parts_mut} will interact with msan? Note that these are all functions that serve to indicate that memory is readable/writable but don't actually read to the memory, so msan won't immediately trigger when they are called. However, I do think we should reserve the right for these functions to call __msan_check_mem_is_initialized, perhaps in some future opt-in or perhaps automatically. At a minimum, we need to document that calling assume_init and the like will NOT call __msan_unpoison.
Similarly, will we reserve the right for pointer-to-reference conversion (unsafe { &*ptr } and unsafe { &mut *p }) to immediately trigger msan via __msan_check_mem_is_initialized in the future, in some mode, perhaps the default?
To address these last two points, we could say in the Memory Sanitizer documentation that, although it is not the case now, any construction of a reference to uninitialized memory may trigger MSAN at some point in the future.

The above is specifically regarding MSAN but I guess analogous concerns for ASAN at least.

rcvalle commented 3 months ago

Is there any concern or anything that the community would like to see implemented before their stabilization?

* `cfg(sanitizer = "known-but-unstable-sanitizer")` should trigger a warning that can be disabled on non-Nightly Rust, instead of triggering the "use of an unstable feature". Otherwise crates that want to support stable Rust need to add feature flags or equivalent to guard their `#[cfg(sanizier)]` usage whenever they do something to support an unstable sanitizer.

* `cfg(sanitizer = "unknown-sanitizer")` should trigger a warning that can be disabled on non-Nightly Rust, for the same reason. Perhaps it should be a separate lint.

Do you have an example of when it would be need that #[cfg(sanitize = "sanitizer")] could not be used instead? If the sanitizer is not stable yet, it wouldn't be enabled and the code not compiled/evaluated by non-nightly builds of the compiler. Would it be for warning purposes only?

rcvalle commented 3 months ago

Is there any concern or anything that the community would like to see implemented before their stabilization?

We are lacking an official way to __msan_unpoison, which is necessary for many things that skip libc and directly invoke syscalls, e.g. via inline assembly or libc::syscall or equivalent. Similarly it is needed for inline assembly because MSAN (at least with Clang's inline assembly) quite reasonably doesn't track writes via inline assembly. Currently I just declare __msan_unpoison as extern "C" and call it. Ideally there would be an official API for this.

I've been working on this for the past few days and have an early/draft implementation of a create that provides (safe) interfaces and FFI bindings for the sanitizers interfaces (see https://crates.io/crates/sanitizers and https://github.com/rcvalle/rust-crate-sanitizers). It currently reflects the current state of the sanitizers interfaces API and documentation, and I think the crate and the sanitizers interfaces themselves would benefit from some standardization of their API and documentation and is something I'd like to improve in the future. Any contributions are greatly appreciated!

When sanitizers are active, what should we assume about how functions like MaybeUninit::{assume_init, assume_init_drop, assume_init_mut, assume_init_ref, slice::{from_raw_parts,from_raw_parts_mut} will interact with msan? Note that these are all functions that serve to indicate that memory is readable/writable but don't actually read to the memory, so msan won't immediately trigger when they are called. However, I do think we should reserve the right for these functions to call __msan_check_mem_is_initialized, perhaps in some future opt-in or perhaps automatically. At a minimum, we need to document that calling assume_init and the like will NOT call __msan_unpoison.

Similarly, will we reserve the right for pointer-to-reference conversion (unsafe { &*ptr } and unsafe { &mut *p }) to immediately trigger msan via __msan_check_mem_is_initialized in the future, in some mode, perhaps the default?

To address these last two points, we could say in the Memory Sanitizer documentation that, although it is not the case now, any construction of a reference to uninitialized memory may trigger MSAN at some point in the future.

The above is specifically regarding MSAN but I guess analogous concerns for ASAN at least.

Thanks for pointing this out! I don't know the answer for this yet, but I think we could definitely reserve the right to (as you suggested) implement additional/specific checks for Rust in the future, similarly to how we could improve these sanitizers generally, and these would not be classified as breaking changes but improvements. What do you think?

rorth commented 2 months ago

There are issues with the entry for x86_64-pc-solaris (and also x86_64-unknown-illumos):

While ASan is supported on Solaris (I should know, I did the port), right now it's 32-bit-only on both x86_64 and sparcv9 (the latter only with gcc, clang is buggy here).
CFI is not supported in upstream compiler-rt, but I read that rust cfi support doesn't require libclang_rt.cfi-*.a.
TSan isn't supported upstream either: PR #112039 that initially enabled it is mistaken: the reference to config-ix.cmake is irrelevant since while COMPILER_RT_TSAN_HAS_STATIC_RUNTIME may be TRUE (for whatever reason), COMPILER_RT_HAS_TSAN is still FALSE. To the best of my knowledge there's never been a tsan port to Solaris/Illumos; certainly there's none upstream.

davidtwco commented 2 months ago

Do you have an example of when it would be need that #[cfg(sanitize = "sanitizer")] could not be used instead? If the sanitizer is not stable yet, it wouldn't be enabled and the code not compiled/evaluated by non-nightly builds of the compiler. Would it be for warning purposes only?

I think what @briansmith is imagining is a scenario where a crate has cfgs for unstable sanitisers (which that crates supports on nightly), but because of those cfgs, the crate now has warnings on stable. On stable, these cfgs would never evaluate to true, so it would be useful to be able to silence the "what is this sanitizer" warnings since you know it's just a yet-to-be-stabilized sanitizer.

briansmith commented 2 months ago

I think what @briansmith is imagining is a scenario where a crate has cfgs for unstable sanitisers (which that crates supports on nightly), but because of those cfgs, the crate now has warnings on stable. On stable, these cfgs would never evaluate to true, so it would be useful to be able to silence the "what is this sanitizer" warnings since you know it's just a yet-to-be-stabilized sanitizer.

Exactly.

rust-lang / rust