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: add missing null check #40598

Closed
wants to merge 1 commit into from
Closed

crypto: add missing null check #40598

wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

Add null check before using result of
ERR_reason_error_string. Coverity reported as an issue
and we seem to do a null check in other places we call
the function.

Signed-off-by: Michael Dawson mdawson@devrus.com

@mhdawson mhdawson requested a review from jasnell October 25, 2021 20:36
@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. needs-ci PRs that need a full CI run. labels Oct 25, 2021
@mhdawson
Copy link
Member Author

@jasnell not sure if a blank error message like I've used is the best we can do or if it would be good to say something like "failed to get message for error". What do you think?

@cjihrig
Copy link
Contributor

cjihrig commented Oct 25, 2021

I don't think we should fall back to the empty string here.

@jasnell
Copy link
Member

jasnell commented Oct 26, 2021

Something like "Unknown error" or "Unspecified error" may work. But yeah, the empty string is not ideal

@mhdawson
Copy link
Member Author

@cjihrig, @jasnell updated to "Unknown error"

tniessen
tniessen reviewed Oct 26, 2021
View reviewed changes
src/crypto/crypto_context.cc
@@ -1037,6 +1037,8 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
// TODO(@jasnell): Should this use ThrowCryptoError?
unsigned long err = ERR_get_error(); // NOLINT(runtime/int)
const char* str = ERR_reason_error_string(err);
str = str != nullptr ? str : "Unknown error";
Copy link
Member

Choose a reason for hiding this comment

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

ERR_reason_error_string should only return nullptr if ERR_get_error returned 0, indicating no error.

If there is a guarantee that ERR_get_error will not return 0, this should probably be a CHECK_NOT_NULL(str) instead. (But I don't know if that's the case.)

Copy link
Member Author

@mhdawson mhdawson Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@tniessen I did wonder about that, but I checked other places that ERR_reason_error_string was called in the Node.js code base and in those places (I think there were 2 others) the code did a nullptr check.

@VoltrexKeyva VoltrexKeyva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Oct 27, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 27, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@github-actions github-actions bot mentioned this pull request Oct 28, 2021
Open
30 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
This was referenced Oct 29, 2021
Open
Open
Open
Open
@nodejs-github-bot

This comment has been minimized.

Sign in to view
This was referenced Nov 4, 2021
Open
Open
Open
@github-actions github-actions bot mentioned this pull request Nov 19, 2021
Open
31 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
This was referenced Nov 20, 2021
Open
Open
Open
@nodejs-github-bot

This comment has been minimized.

Sign in to view
This was referenced Nov 23, 2021
Open
Open
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@github-actions github-actions bot mentioned this pull request Nov 25, 2021
Open
38 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Nov 26, 2021
Open
33 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Nov 27, 2021
Open
Open
Open
@mhdawson
Copy link
Member Author

Looking at the CI job 41147 all jobs passed including the one that the checks on the PR show as failed. Not sure why that is but since CI is green will land.

mhdawson added a commit that referenced this pull request Nov 29, 2021
@mhdawson
crypto: add missing null check
8e7fd72 
Add null check before using result of
ERR_reason_error_string. Coverity reported as an issue
and we seem to do a null check in other places we call
the function.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #40598
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@mhdawson
Copy link
Member Author

Landed in 8e7fd72

@mhdawson mhdawson closed this Nov 29, 2021
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@mhdawson @danielleadams
crypto: add missing null check
99c1a69 
Add null check before using result of
ERR_reason_error_string. Coverity reported as an issue
and we seem to do a null check in other places we call
the function.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #40598
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@mhdawson @danielleadams
crypto: add missing null check
c757fa5 
Add null check before using result of
ERR_reason_error_string. Coverity reported as an issue
and we seem to do a null check in other places we call
the function.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #40598
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 14, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@mhdawson @danielleadams
crypto: add missing null check
adcc29c 
Add null check before using result of
ERR_reason_error_string. Coverity reported as an issue
and we seem to do a null check in other places we call
the function.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #40598
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@mhdawson @danielleadams
crypto: add missing null check
3055ede 
Add null check before using result of
ERR_reason_error_string. Coverity reported as an issue
and we seem to do a null check in other places we call
the function.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #40598
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
6 participants
@mhdawson @cjihrig @jasnell @nodejs-github-bot @tniessen @VoltrexKeyva