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

Test getrandom_uninit() with initially-uninitialized buffer. #462

Closed
wants to merge 1 commit into from

Conversation

briansmith
Copy link
Contributor

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/test-uninit branch 2 times, most recently from 6bdba5c to 1175729 Compare June 6, 2024 23:36
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
Copy link
Contributor Author

@josephlr @newpavlov Could we review and merge this PR, which doesn't have the MSAN-specific changes to it? I have other testing enhancements that depend on this, that I don't want to block on the MSAN stuff.

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
@josephlr
Copy link
Member

@josephlr @newpavlov Could we review and merge this PR, which doesn't have the MSAN-specific changes to it? I have other testing enhancements that depend on this, that I don't want to block on the MSAN stuff.

Sorry for the delay. The testing code is currently a mess. #471 should fix things to stop relying on weird module renaming hacks. I also included the ideas form this CL into that one. If #471 looks good to you, feel free to close this.

@newpavlov
Copy link
Member

Closing in favor of #505.

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

Successfully merging this pull request may close these issues.

3 participants