google / mundane

Mundane is a Rust cryptography library backed by BoringSSL that is difficult to misuse, ergonomic, and performant (in that order).
MIT License
1.07k stars 46 forks source link

Run tests with MSan #15

Open joshlf opened 5 years ago

joshlf commented 5 years ago

UPDATE: ASan has been implemented in https://github.com/google/mundane/commit/0148297bf1c5bb8ccc8acbc7abcc15cb4b53d6ee. We should still run tests with MSan, although it's less important than ASan.

=== old text ===

We should enable ASan and MSan when running cargo test in order to catch issues with our use of the BoringSSL API. ASan should help catch issues with reference counting, allocation, and freeing, while MSan should help catch issues with memory initialization.

brycx commented 5 years ago

There seem to be problems when running cargo test under MSan and I've never managed to do it successfully myself. Could Miri account for some of the things MSan would be able to check for?

davidben commented 5 years ago

For refcounts and allocation bits, ASan (with leak-checking enabled) should be sufficient. But MSan is also generally a good idea.

I'm not familiar with Miri, but given that it instruments Rust's MIR, I doubt it would do much. The interesting stuff is going to be at the Rust <-> C boundary, so you need to instrument both compatibly.

joshlf commented 5 years ago

ASan is the bigger deal here. Getting ASan alone working would be huge.

Shnatsel commented 5 years ago

What are the work items here? Why does RUSTFLAGS="-Z sanitizer=address" cargo test --target x86_64-unknown-linux-gnu not suffice?

Shnatsel commented 5 years ago

https://github.com/rust-lang/rust/issues/53945 will a problem for running tests in release mode but there is a workaround, and if you're running in debug mode you're fine.

joshlf commented 5 years ago

@Shnatsel

What are the work items here? Why does RUSTFLAGS="-Z sanitizer=address" cargo test --target x86_64-unknown-linux-gnu not suffice?

If it's that simple, then that's great. I know nothing about sanitizers in Rust, so it's very possible that I just wasn't aware it was this straightforward.

https://github.com/rust-lang/rust/issues/53945 will a problem for running tests in release mode but there is a workaround, and if you're running in debug mode you're fine.

Doing it just in debug mode for the time being should be fine.

Shnatsel commented 5 years ago

Yup, it's that straightforward.

Sometimes ASAN produces what I assume to be false positives, complaining about "ODR violation" which seems to be an error condition exclusive to C++. This is how to suppress it:

// suppress ASAN false positives
const ASAN_DEFAULT_OPTIONS: &'static [u8] = b"detect_odr_violation=1\0";
#[no_mangle]
pub extern "C" fn __asan_default_options() -> *const u8 {
    ASAN_DEFAULT_OPTIONS as *const [u8] as *const u8
}

https://github.com/japaric/rust-san contains more documentation and recipes, AFAIK it's the hub for sanitizer work in Rust.

joshlf commented 5 years ago

Update: We should be able to have this run all the time by editing test.sh.

joshlf commented 5 years ago

ASan has been implemented here: https://github.com/google/mundane/commit/0148297bf1c5bb8ccc8acbc7abcc15cb4b53d6ee.

I'm going to leave this open in case somebody wants to figure out how to get MSan working.