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

Add cosistency test for fcrypt. #81

Merged
merged 3 commits into from
Feb 6, 2019
Merged

Conversation

besser82
Copy link
Owner

@besser82 besser82 commented Feb 4, 2019

As suggested / proposed in #79.

@besser82 besser82 requested a review from zackw February 4, 2019 21:50
@besser82 besser82 force-pushed the besser82/fcrypt_consistency branch 2 times, most recently from afb276b to 75270ec Compare February 5, 2019 11:08
@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #81 into develop will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #81      +/-   ##
===========================================
- Coverage    96.46%   96.39%   -0.07%     
===========================================
  Files           32       32              
  Lines         3109     3109              
===========================================
- Hits          2999     2997       -2     
- Misses         110      112       +2
Impacted Files Coverage Δ
crypt-nthash.c 85.71% <0%> (-3.58%) ⬇️
crypt-sunmd5.c 98.14% <0%> (-0.93%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29fec31...75270ec. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #81 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #81   +/-   ##
========================================
  Coverage    96.39%   96.39%           
========================================
  Files           32       32           
  Lines         3109     3109           
========================================
  Hits          2997     2997           
  Misses         112      112
Impacted Files Coverage Δ
alg-yescrypt-opt.c 97.58% <0%> (-0.22%) ⬇️
crypt-pbkdf1-sha1.c 98.64% <0%> (+1.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41c1f65...0a7ada6. Read the comment docs.

@besser82
Copy link
Owner Author

besser82 commented Feb 5, 2019

Looks like we need to get #62 working before we can merge this one, as we cannot run fcrypt in a parallel thread aside of crypt. Running them sequentially makes the valgrind test with Clang timeout. :/

@zackw
Copy link
Collaborator

zackw commented Feb 5, 2019

I have just pushed a patch on top of this branch which reorganizes the work done by the test to make better use of parallelism and also eliminate some redundant operations. I hope this make it possible to test fcrypt on top of the others without having to increase the timeouts again.

Assuming CI goes green, I'm good with the branch as of this patch. Please feel free to squash my change and/or make further adjustments on top.

besser82 and others added 2 commits February 6, 2019 21:30
fcrypt, like crypt, is not thread-safe, so have the main thread call
crypt() and then fcrypt() for each test case; meanwhile, another
thread calls crypt_r() and then crypt_rn(), and a third thread calls
crypt_ra() and then repeats the call with the output hash instead of
the original setting string.  All are checked solely against the
expected hash from test-crypt-kat.inc; transitivity means there’s no
point in testing them against each other once they passed that check.
Also, errors are reported immediately instead of being buffered up,
which means we can scrap a bunch of memory management code.

The collisions test, meanwhile, is moved to test-crypt-kat-gen.py.
If we know that the only collisions among the *expected* hashes are
the ones we anticipate, then checking the freshly generated hashes
against the expected hashes is sufficient to detect new collisions.
So we don’t need to do any of that work at build time.
@besser82 besser82 force-pushed the besser82/fcrypt_consistency branch from e3a6d4e to a864b03 Compare February 6, 2019 20:34
@besser82
Copy link
Owner Author

besser82 commented Feb 6, 2019

@zackw I've corrected the way CFLAGS are set.

For some reason the code generated by Clang is significantly slower
than the code generated by GCC, when run under Valgrind.  It’s only
about ten additional minutes of wall-clock time, but it makes the test
suite trip not only Travis’s documented ten-minute no-output
timeout (which can be worked around), but their undocumented
45?-minute no-matter-what timeout.

Switch the Clang checked build on Travis to use ASan+UBSan instead of
Valgrind.  This is much faster and may also detect a different subset
of potential bugs.
@besser82 besser82 force-pushed the besser82/fcrypt_consistency branch from a864b03 to 0a7ada6 Compare February 6, 2019 21:00
@besser82
Copy link
Owner Author

besser82 commented Feb 6, 2019

@zackw Added travis_wait 60 to make check.

@besser82
Copy link
Owner Author

besser82 commented Feb 6, 2019

Will merge on CLI as soon as Travis turns green.

@besser82 besser82 merged commit 0a7ada6 into develop Feb 6, 2019
@besser82 besser82 deleted the besser82/fcrypt_consistency branch February 6, 2019 22:24
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.

2 participants