Skip to content

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

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
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

tmeisenh
Copy link
Contributor

@tmeisenh tmeisenh commented Jan 19, 2017 •
edited by gibfahn
Loading

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 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
Copy link
Contributor Author

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

@mscdex
Copy link
Contributor

mscdex commented Jan 19, 2017

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

@tmeisenh
Copy link
Contributor Author

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

@gibfahn
Copy link
Member

gibfahn commented Jan 19, 2017 •
edited
Loading

@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
bnoordhuis approved these changes Jan 19, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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

@tmeisenh
Copy link
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
Copy link
Member

Trott commented Jan 20, 2017

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.

@tmeisenh
crypto: return the retval of HMAC_Update
2dc9920 
Fixes coverity scan issue 55489.
@tmeisenh
Copy link
Contributor Author

Updated commit message

sam-github
sam-github reviewed Jan 20, 2017
View reviewed changes
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Jan 20, 2017

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
Copy link
Contributor

Landed in 5ea98fb, thanks.

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

PR-URL: #10891
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Brian White <[email protected]>
targos pushed a commit that referenced this pull request Jan 28, 2017
@tmeisenh @targos
crypto: return the retval of HMAC_Update
e691fb8 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Brian White <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@tmeisenh
crypto: return the retval of HMAC_Update
6687b95 
Fixes coverity scan issue 55489.

PR-URL: nodejs#10891
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Brian White <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@tmeisenh
crypto: return the retval of HMAC_Update
527ce0d 
Fixes coverity scan issue 55489.

PR-URL: nodejs#10891
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Brian White <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
@tmeisenh @jasnell
crypto: return the retval of HMAC_Update
0277eea 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Brian White <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
@tmeisenh @jasnell
crypto: return the retval of HMAC_Update
076d56e 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@tmeisenh @MylesBorins
crypto: return the retval of HMAC_Update
b662c11 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@tmeisenh @MylesBorins
crypto: return the retval of HMAC_Update
c9a92ff 
Fixes coverity scan issue 55489.

PR-URL: #10891
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@sam-github 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

@tmeisenh @mscdex @gibfahn @Trott @sam-github @bnoordhuis @jasnell @cjihrig @nodejs-github-bot