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.

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

Android/Linux: Support msan; unpoison output of getrandom syscall. #463

Closed
wants to merge 2 commits into from

Conversation

briansmith
Copy link
Contributor

See the added comment for details.

None of the tests are testing `getrandom_uninit()` directly, but
instead it is marked as covered because `getrandom()` forwards to
it. However, this means we're never testing an uninitialized input
to `getrandom_uninit()`. Fix that.
@briansmith briansmith force-pushed the b/msan-1 branch 3 times, most recently from e791121 to f4a95fe Compare June 7, 2024 00:04
@josephlr
Copy link
Member

josephlr commented Jun 7, 2024

If we moved away from using MaybeUninit (#454) would we need any of this?

@briansmith
Copy link
Contributor Author

If we moved away from using MaybeUninit (#454) would we need any of this?

I will comment in that issue. In short, keeping the MaybeUninit API seems useful for allowing us to use Memory Sanitizer to test that our implementations are working correctly.

@briansmith briansmith marked this pull request as ready for review June 7, 2024 00:21
@briansmith
Copy link
Contributor Author

Note that this incorporates the changes in #462.

@briansmith
Copy link
Contributor Author

Also note that the tests added in #462 fail when with Memory Sanitizer enabled before these changes, but pass after these changes, using the command line added to the crate-level documentation.

@josephlr
Copy link
Member

josephlr commented Jun 7, 2024

Aren't we supposed to check for sanitizers like #[cfg(sanitize = "memory")]. If there's some sort of bug with the unstable cfg_sanitize feature, I would prefer that being fixed before adding this.

Comment on lines 115 to 131
#[allow(unused_variables)]
fn check_initialized(buf: &[MaybeUninit<u8>]) {
#[cfg(feature = "unstable-sanitize")]
{
// XXX: `#![feature(cfg_sanitize)]` doesn't enable the feature gate correctly.
// #[cfg(sanitize = "memory")]
{
use core::ffi::c_void;
extern "C" {
// void __msan_check_mem_is_initialized(const volatile void *x, size_t size);
fn __msan_check_mem_is_initialized(x: *const c_void, size: usize);
}
unsafe {
__msan_check_mem_is_initialized(buf.as_ptr().cast::<c_void>(), buf.len());
}
}
}
}
Copy link
Member

@josephlr josephlr Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this complexity, can we just do something in the huge tests which reads the huge buffer. Then MSAN will fail if we've messed up, but we don't need to have any cfg-specific code in our tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could maybe do the same thing we do for the large tests? I didn't want to try understanding what the large test is doing, so this was easier :) Also I was curious about whether __msan_check_mem_is_initialized would work.

@briansmith
Copy link
Contributor Author

Aren't we supposed to check for sanitizers like #[cfg(sanitize = "memory")]. If there's some sort of bug with the unstable cfg_sanitize feature, I would prefer that being fixed before adding this.

Yes, but I couldn't get it to work. You can see that the #[cfg(sanitize = "memory")] is already in this PR; it's just commented out. Maybe I am overlooking some mistake I made when testing it? Regardless, we'd also we'd still need a feature flag to opt into unstable/Nightly-only features, so having the #[cfg(sanitize = "memory")] working won't be a substantial change.

@@ -52,6 +52,8 @@ rustc-dep-of-std = [
"libc/rustc-dep-of-std",
"wasi/rustc-dep-of-std",
]
# Enable support for sanitizers; Requires Rust feature `cfg_sanitize`.
unstable-sanitize = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it better to use a configuration flag for this instead of this crate feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. The good thing about using a feature flag is that other crates can then trigger it automatically with their own feature flag depending on it. That's what I've been planning to do in ring, for example.

@briansmith briansmith force-pushed the b/msan-1 branch 4 times, most recently from bd7ea32 to de26fa5 Compare June 7, 2024 21:30
@briansmith
Copy link
Contributor Author

PR #467 adds a commit that disables the poison call to show that the new msan CI jobs work. See the job fail at https://github.com/rust-random/getrandom/actions/runs/9423708743/job/25962648547, with output:

running 9 tests
test common::test_huge ... ok
test common::test_huge_uninit ... ok
test common::test_large ... ok
test common::test_large_uninit ... ok
==2308==WARNING: MemorySanitizer: use-of-uninitialized-value
test common::test_zero ... ok

[snip]

SUMMARY: MemorySanitizer: use-of-uninitialized-value (/home/runner/work/getrandom/getrandom/target/x86_64-unknown-linux-gnu/debug/deps/normal-bab58a9b7cd91267+0x204925) (BuildId: 76ce175ae2099554) 
Exiting
error: test failed, to rerun pass `--test normal`

Caused by:
  process didn't exit successfully: `/home/runner/work/getrandom/getrandom/target/x86_64-unknown-linux-gnu/debug/deps/normal-bab58a9b7cd9[126](https://github.com/rust-random/getrandom/actions/runs/9423708743/job/25962648547#step:5:127)7` (exit status: 1)
note: test exited abnormally; to see the full output pass --nocapture to the harness.
Error: Process completed with exit code 1.

@briansmith
Copy link
Contributor Author

Aren't we supposed to check for sanitizers like #[cfg(sanitize = "memory")]. If there's some sort of bug with the unstable cfg_sanitize feature, I would prefer that being fixed before adding this.

There isn't a bug; I just was missing the feature gate in the integration tests and misread the output. All fixed now.

josephlr added a commit that referenced this pull request Jun 10, 2024
This stops using weird include hacks to reuse code. Instead, we move the
code to a nomral `tests.rs` file and use helper functions to avoid code
duplication. This also simplifies our testing of the custom RNGs.
Before, we had to disable the "normal" tests to test the custom RNG.
Now, the custom RNG is "good enough" to simply pass the tests.

I also added direct testing for the `uninit` methods and verified that
```
RUSTFLAGS="-Zsanitizer=memory" cargo +nightly test -Zbuild-std --target=x86_64-unknown-linux-gnu
```
fails (but passes when #463 is added), so we are actually testing the
right stuff here. So this can replace #462

Signed-off-by: Joe Richey <[email protected]>
@josephlr josephlr mentioned this pull request Jun 10, 2024
@newpavlov
Copy link
Member

Closing in favor of #521. It uses less granular unpoisoning in getrandom_uninit which is not target specific.

@newpavlov newpavlov closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants