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

All test cases use buffers that are multiples of the word size #290

Closed
briansmith opened this issue Oct 12, 2022 · 1 comment · Fixed by #304
Closed

All test cases use buffers that are multiples of the word size #290

briansmith opened this issue Oct 12, 2022 · 1 comment · Fixed by #304
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@briansmith
Copy link
Contributor

briansmith commented Oct 12, 2022

Consider this change to rdrand_exact():

@@ -91,7 +91,7 @@ unsafe fn rdrand_exact(dest: &mut [u8]) -> Result<(), Error> {
     let tail = chunks.into_remainder();
     let n = tail.len();
     if n > 0 {
-        tail.copy_from_slice(&rdrand()?[..n]);
+        unreachable!()
     }
     Ok(())
 }

I would expect at least one test to fail with this modification, but they all pass, because every test buffer is a multiple of the word size. We should have tests for this branch.

@josephlr
Copy link
Member

Thanks! This is a good catch, we should add (a bunch) of tests with weird-length buffers.

@josephlr josephlr added enhancement New feature or request good first issue Good for newcomers labels Oct 13, 2022
josephlr added a commit that referenced this issue Oct 22, 2022
This PR adds some tests which make sure calls to getrandom (for both small and large buffers) "look" random.

While we could certainly add more complicated randomness tests, these simple tests are:
  - Very easy to understand
  - Don't require any external crates
  - Makes sure we aren't doing something obviously stupid like
    - forgetting [these lines](https://github.com/rust-random/getrandom/blob/bd0654fe70980583e51573e755bafa3b2f8342d9/src/rdrand.rs#L91-L95)
    - failing to initialize every other byte
    - initializing some significant fraction of bytes with a constant

As this tests all buffer sizes from 1 to 64, it also fixes #290.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants