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 cases where pbkdf2_hmac panics #23

Closed
briansmith opened this issue Sep 25, 2015 · 5 comments
Closed

Test cases where pbkdf2_hmac panics #23

briansmith opened this issue Sep 25, 2015 · 5 comments

Comments

@briansmith
Copy link
Owner

Currently there are some test cases in pbkdf2_tests.txt that are commented out because they are invalid inputs for ring's PBKDF2 module. In particular, they request an output longer than the output length of the the digest function being used. We should turn those test cases into the equivalent of [should_panic]. AFAICT, this means we need to:

  1. Uncomment those test cases.
  2. Annotate those test cases with "FAILS: OUTPUT_LARGER_THAN_DIGEST_OUTPUT".
  3. Change the testing code so that for the cases marked "FAILS: OUTPUT_LARGER_THAN_DIGEST_OUTPUT", a task gets spawned that does the test case. We have to wait for the task to end and then verify that it was ended due to a panic!. In other words, we need to create the equivalent of a [should_panic] test.

Additionally, we should add more test cases, particularly a test case for iterations == 0, as BoringSSL has (had) a bug when interations == 0.

AFAICT, this is complicated by rust-lang/rust#25869, as I believe we cannot do [should_panic] tests or create a data-driven alternative to [should_panic] on Win32 since Rust on Win32 doesn't implement panic! correctly; `panic!`` just kills the entire process. We should probably just work around that by not running these tests on Win32.

@briansmith
Copy link
Owner Author

#[should_panic] works on all platforms now and is already being used by some tests. So we can definitely do this now.

@joshuata
Copy link
Contributor

I believe this was resolved by f87e2e8 and a316606

@briansmith
Copy link
Owner Author

Yes, you're right, given the description above. However, we should add a #[should_panic] test for the case where iterations == 0, which is the remaining case that can panic.

@joshuata
Copy link
Contributor

Okay. I will get on that ASAP.

@briansmith
Copy link
Owner Author

Fixed in #466. Thanks again, @joshuata.

@briansmith briansmith added this to the 0.7.n milestone Mar 9, 2017
@briansmith briansmith modified the milestones: 0.7.n, 0.7.3 Mar 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants