FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: return result of openssl HMAC_Update instead of just true. by tmeisenh · Pull Request #10891 · nodejs/node · GitHub

/ node Public

crypto: return result of openssl HMAC_Update instead of just true. - #10891

Closed
tmeisenh wants to merge 1 commit into
nodejs:masterfrom
tmeisenh:crypto_fixes
ConversationCommits (1)ChecksFiles changed
Closed

crypto: return result of openssl HMAC_Update instead of just true.#10891
tmeisenh wants to merge 1 commit into
nodejs:masterfrom
tmeisenh:crypto_fixes

Conversation

tmeisenh commented Jan 19, 2017 •
edited by gibfahn
Loading

Copy link
Copy Markdown
Contributor

Addresses coverity scan issue 55489

The existing tests pass but I did not add any new tests. I did not see any existing tests around error handling of openssl functions nor any test harness to allow you to do so.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lts-watch-v6.x labels Jan 19, 2017

tmeisenh commented Jan 19, 2017

Copy link
Copy Markdown
Contributor Author

Updated commit (force push on my branch) just updates original commit message to reference the issue number.

mscdex commented Jan 19, 2017

Copy link
Copy Markdown
Contributor

LGTM if CI is ok: https://ci.nodejs.org/job/node-test-pull-request/5941/

tmeisenh commented Jan 19, 2017

Copy link
Copy Markdown
Contributor Author

I must be missing something b/c when I review the output of test/arm I don't see a failure...

gibfahn commented Jan 19, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@tmeisenh It's an issue with the github bot that sends the results back, if it's green on ci.nodejs.org (which it is), then CI passed.

CI is green

bnoordhuis approved these changes Jan 19, 2017
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM but the commit log's status line should be <= 50 characters.

tmeisenh commented Jan 19, 2017

Copy link
Copy Markdown
Contributor Author

@gibfahn Awesome, thanks for the explanation.
@bnoordhuis nice catch on the commit log exceeding the recommended length. I can alter that if necessary.

Trott commented Jan 20, 2017

Copy link
Copy Markdown
Member

nice catch on the commit log exceeding the recommended length. I can alter that if necessary.

If you wouldn't mind amending the commit message, you'll save whoever lands this a few keystrokes. But if you don't update it, whoever lands it can update it for you, so either way, really.

crypto: return the retval of HMAC_Update
2dc9920 
Fixes coverity scan issue 55489.

tmeisenh commented Jan 20, 2017

Copy link
Copy Markdown
Contributor Author

Updated commit message

sam-github reviewed Jan 20, 2017
View reviewed changes
Comment thread src/node_crypto.cc
HMAC_Update(&ctx_, reinterpret_cast<const unsigned char*>(data), len);
return true;
int r = HMAC_Update(&ctx_, reinterpret_cast<const unsigned char*>(data), len);
return r == 1;

sam-github Jan 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks like it can only occur with engines: openssl/openssl@87d52468aa600e023, so untestable.

sam-github approved these changes Jan 20, 2017
View reviewed changes

sam-github left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK, debugged, it does look the digests return 1 on success, and always do this for software digests, so LGTM

Trott commented Jan 20, 2017

Copy link
Copy Markdown
Member

Running CI again only because the pushing of the amended commit message theoretically may have accidentally included a code change or something. Just being thorough.

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

sam-github commented Jan 24, 2017

Copy link
Copy Markdown
Contributor

Landed in 5ea98fb, thanks.

sam-github closed this Jan 24, 2017
sam-github pushed a commit that referenced this pull request Jan 24, 2017
crypto: return the retval of HMAC_Update
5ea98fb 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
targos pushed a commit that referenced this pull request Jan 28, 2017
crypto: return the retval of HMAC_Update
e691fb8 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
italoacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
crypto: return the retval of HMAC_Update
6687b95 
Fixes coverity scan issue 55489.

PR-URL: nodejs#10891
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
crypto: return the retval of HMAC_Update
527ce0d 
Fixes coverity scan issue 55489.

PR-URL: nodejs#10891
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
crypto: return the retval of HMAC_Update
0277eea 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
crypto: return the retval of HMAC_Update
076d56e 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
crypto: return the retval of HMAC_Update
b662c11 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
crypto: return the retval of HMAC_Update
c9a92ff 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
+1 more reviewer
sam-github sam-github approved these changes
Reviewers whose approvals may not affect merge requirements

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL