search

rust-random / getrandom

A small cross-platform library for retrieving random data from (operating) system source
Apache License 2.0
264 stars 166 forks source link

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

Open briansmith opened 4 weeks ago

briansmith commented 4 weeks ago

See the added comment for details.

josephlr commented 4 weeks ago

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

briansmith commented 4 weeks ago

If we moved away from using MaybeUninit (https://github.com/rust-random/getrandom/issues/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 commented 4 weeks ago

Note that this incorporates the changes in #462.

briansmith commented 4 weeks ago

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 commented 4 weeks ago

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.

briansmith commented 4 weeks ago

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.

briansmith commented 3 weeks ago

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 commented 3 weeks ago

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.