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.

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

crypto: replace goto SSL_CTX_use_certificate_chain #23113

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Sep 27, 2018

This commit removes the goto statements in SSL_CTX_use_certificate_chain
by using a unique_ptr.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@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. labels Sep 27, 2018
@danbev
Copy link
Contributor Author

danbev commented Sep 27, 2018

X509* extra = nullptr;
int ret = 0;
OnScopeLeave cleanup([&extra, &ret] {
if (ret)
Copy link
Member

Choose a reason for hiding this comment

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

There is a potential footgun here. Someone someday might look at line 686 and see this:

ret = SSL_CTX_use_certificate_chain(...);
return ret;

And change that to:

return SSL_CTX_use_certificate_chain(...);

Without realizing that the assignment to ret is necessary because of its side effects. Probably also easy to miss in code review.

Copy link
Member

Choose a reason for hiding this comment

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

I think we could really do a variant here that uses a smart pointer and releases it if necessary; it’s just not trivial to translate the code to that…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about the late response on this. I'll take another stab at this today. Thanks

@danbev danbev added the wip Issues and PRs that are still a work in progress. label Oct 3, 2018
This commit removes the goto statements in SSL_CTX_use_certificate_chain
by using a unique_ptr.
@danbev danbev removed the wip Issues and PRs that are still a work in progress. label Oct 4, 2018
@danbev
Copy link
Contributor Author

danbev commented Oct 4, 2018

Updated CI: https://ci.nodejs.org/job/node-test-pull-request/17623/ (:heavy_check_mark:)

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 4, 2018
@danbev
Copy link
Contributor Author

danbev commented Oct 8, 2018

Landed in 8d218a9.

@danbev danbev closed this Oct 8, 2018
@danbev danbev deleted the crypto_cert_chain_goto branch October 8, 2018 03:56
danbev added a commit that referenced this pull request Oct 8, 2018
This commit removes the goto statements in SSL_CTX_use_certificate_chain
by using a unique_ptr.

PR-URL: #23113
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 10, 2018
This commit removes the goto statements in SSL_CTX_use_certificate_chain
by using a unique_ptr.

PR-URL: #23113
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
This commit removes the goto statements in SSL_CTX_use_certificate_chain
by using a unique_ptr.

PR-URL: #23113
Reviewed-By: Anna Henningsen <anna@addaleax.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
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
Development

Successfully merging this pull request may close these issues.

5 participants