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

crypto: better error message when calling digest twice on a hash #6042

Closed
wants to merge 1 commit into from
Closed

crypto: better error message when calling digest twice on a hash #6042

wants to merge 1 commit into from

Conversation

calvinmetcalf
Copy link
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?

I'm getting an error on the test-tick-processor test, seems unrelated

  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

Description of change

calling digest twice on a hash used to give an unhelpful error about the hash not being initialized, this fixes that

@calvinmetcalf calvinmetcalf changed the title [crypto] better error message when calling digest twice on a hash crypto: better error message when calling digest twice on a hash Apr 4, 2016
@calvinmetcalf calvinmetcalf mentioned this pull request Apr 4, 2016
Closed
@fanatid
Copy link
Contributor

fanatid commented Apr 4, 2016

It would be nice to have same for Hmac.
BTW, right now second call hmac.digest don't throwing error, but docs says that it should: https://nodejs.org/api/crypto.html#crypto_hmac_digest_encoding (Multiple calls to hmac.digest() will result in an error being thrown.)

@calvinmetcalf
Copy link
Contributor Author

I'd probably want to put that into a different pull request

mscdex
mscdex reviewed Apr 4, 2016
View reviewed changes
src/node_crypto.cc
@@ -3735,7 +3739,7 @@ void Hash::HashDigest(const FunctionCallbackInfo<Value>& args) {

EVP_DigestFinal_ex(&hash->mdctx_, md_value, &md_len);
EVP_MD_CTX_cleanup(&hash->mdctx_);
hash->initialised_ = false;
hash -> finalized_ = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the spaces before and after the arrow should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@mscdex
Copy link
Contributor

mscdex commented Apr 4, 2016

Perhaps we should also be inserting a similar check for .update()?

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Apr 4, 2016
addaleax
addaleax reviewed Apr 4, 2016
View reviewed changes
src/node_crypto.cc
@@ -3722,6 +3723,9 @@ void Hash::HashDigest(const FunctionCallbackInfo<Value>& args) {
if (!hash->initialised_) {
return env->ThrowError("Not initialized");
}
if (hash->finalized_) {
return env->ThrowError("Digest Already Called");
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for the capitalization here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point fixing

@fanatid
Copy link
Contributor

fanatid commented Apr 4, 2016

why not use only finalized_? (in .update() and .digest())
I don't see sense in initialised_ because Hash can't be refreshed

@jasnell jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. error-handling labels Apr 4, 2016
@calvinmetcalf calvinmetcalf force-pushed the digest-error-msg branch 2 times, most recently from 3b61487 to ba02203 Compare April 5, 2016 11:25
@calvinmetcalf
Copy link
Contributor Author

@fanatid because there could be some way that a bug could cause the hash to be in an inconsistent state and better safe then sorry

@fanatid
Copy link
Contributor

fanatid commented Apr 5, 2016

@calvinmetcalf I think that this line never will be reached because you add error throwing
I don't see how initialised_ can be set to false since you've added finalized_

@calvinmetcalf
Copy link
Contributor Author

@fanatid if this line was never reached

mscdex
mscdex reviewed Apr 5, 2016
View reviewed changes
test/parallel/test-crypto-hash.js
assert.throws(function() {
h3.digest();
},
/Digest Already Called/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Capitalization should be changed here too.

@mscdex
Copy link
Contributor

mscdex commented Apr 5, 2016

I think an additional test for .update() should be added for completeness. Other than that it LGTM if CI is ok with it.

@calvinmetcalf calvinmetcalf force-pushed the digest-error-msg branch 3 times, most recently from 3cda16d to 4d5a452 Compare April 5, 2016 12:49
@jasnell
Copy link
Member

jasnell commented Apr 5, 2016

LGTM pending CI

@mscdex
Copy link
Contributor

mscdex commented Apr 5, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/2159/

@fanatid
Copy link
Contributor

fanatid commented Apr 6, 2016

@calvinmetcalf could you answer for my first question?

this line never will be reached because you add error throwing

mscdex
mscdex reviewed Apr 6, 2016
View reviewed changes
test/parallel/test-crypto-hash.js
h3.digest();
},
/Digest already called/);

Copy link
Contributor

Choose a reason for hiding this comment

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

Linter is complaining about trailing spaces on this line

@calvinmetcalf
Copy link
Contributor Author

@fanatid woops was cut off replying via email, the initialized would only ever be false if for some reason the hash wasn't initialized, which probably not be possible as it is currently set up, but we don't really loose much testing for it and we will prevent problems down the line if some other change allows it to get into a weird state.

@calvinmetcalf
crypto: better error message for createHash
2cb82d2 
calling digest or update on a hash object after digest has been called
now gives a topical error message instead of an error message saying that the
hash failed to initialize.
@jasnell
Copy link
Member

jasnell commented Apr 12, 2016

@mscdex ... ping.. looks like this was updated. PTAL

@mscdex
Copy link
Contributor

mscdex commented Apr 12, 2016

CI again: https://ci.nodejs.org/job/node-test-pull-request/2245/

@jasnell
Copy link
Member

jasnell commented Apr 12, 2016

CI is green

@mscdex
Copy link
Contributor

mscdex commented Apr 12, 2016

LGTM

jasnell pushed a commit that referenced this pull request Apr 18, 2016
@calvinmetcalf @jasnell
crypto: better error message for createHash
1d9451b 
calling digest or update on a hash object after digest has been called
now gives a topical error message instead of an error message saying that the
hash failed to initialize.

PR-URL: #6042
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

Landed in 1d9451b

@jasnell jasnell closed this Apr 18, 2016
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@calvinmetcalf @joelostrowski
crypto: better error message for createHash
5c7c6d1 
calling digest or update on a hash object after digest has been called
now gives a topical error message instead of an error message saying that the
hash failed to initialize.

PR-URL: nodejs#6042
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@calvinmetcalf @jasnell
crypto: better error message for createHash
47cca06 
calling digest or update on a hash object after digest has been called
now gives a topical error message instead of an error message saying that the
hash failed to initialize.

PR-URL: #6042
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@calvinmetcalf @fanatid @mscdex @jasnell @addaleax