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.

Sign up for GitHub

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

Jump to bottom

Full backend for scrypt with tests #31

Merged
merged 5 commits into from
Sep 8, 2018
Merged

Full backend for scrypt with tests #31

merged 5 commits into from
Sep 8, 2018

Conversation

besser82
Copy link
Owner

@besser82 besser82 commented Sep 7, 2018

As mentioned in #27 here comes the backend for scrypt.

@besser82 besser82 requested a review from zackw September 7, 2018 14:33
@codecov-io
Copy link

codecov-io commented Sep 7, 2018 •
edited
Loading

Codecov Report

Merging #31 into develop will increase coverage by 0.21%.
The diff coverage is 89.07%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #31      +/-   ##
===========================================
+ Coverage    88.54%   88.75%   +0.21%     
===========================================
  Files           28       30       +2     
  Lines         3063     3166     +103     
===========================================
+ Hits          2712     2810      +98     
- Misses         351      356       +5
Impacted Files Coverage Δ
alg-yescrypt-opt.c 77.28% <ø> (ø) ⬆️
alg-yescrypt-sha256.c 73.66% <ø> (ø) ⬆️
alg-yescrypt-common.c 63.77% <ø> (+1.49%) ⬆️
crypt-port.h 100% <100%> (ø)
crypt-yescrypt.c 86.48% <81.25%> (ø) ⬆️
crypt-scrypt.c 89.47% <89.47%> (ø)

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 3a3ffba...0a0825b. Read the comment docs.

@zackw
Copy link
Collaborator

zackw commented Sep 7, 2018

I will look at this tomorrow, I can't do it today.

@besser82
Copy link
Owner Author

besser82 commented Sep 7, 2018

np

@besser82 besser82 force-pushed the besser82/scrypt branch 4 times, most recently from 411b0f7 to 5f7223d Compare September 7, 2018 19:49
@besser82 besser82 force-pushed the besser82/scrypt branch 3 times, most recently from df7a44a to bb2a86e Compare September 8, 2018 10:45
zackw
zackw approved these changes Sep 8, 2018
View reviewed changes
Copy link
Collaborator

@zackw zackw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with just one concern regarding XCRYPT_SECURE_MEMSET (see inline comments)

crypt-port.h
}
#define XCRYPT_STRCPY_OR_ABORT(dst, d_size, src) \
_xcrypt_strcpy_or_abort ((char *) dst, (const size_t) d_size, \
(const char *) src)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the casts in the definition of XCRYPT_STRCPY_OR_ABORT necessary? If it's because some callers pass char *, some pass unsigned char *, and some pass void *, instead I suggest making the inline function take void * and const void * arguments instead, and then casting in the body of the function. That way the compiler will at least still catch const-correctness errors.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to go ahead and merge this and then tinker with XCRYPT_STRCPY_OR_ABORT myself.

@zackw zackw merged commit 9ae2acb into develop Sep 8, 2018
@besser82 besser82 deleted the besser82/scrypt branch September 8, 2018 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zackw zackw zackw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@besser82 @codecov-io @zackw