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: make some easy parts 1.1.0-compatible #15348

Closed
wants to merge 2 commits into from

Conversation

davidben
Copy link
Contributor

I'll upload a PR for the rest of it once I've finished fixing up the original PR, but since these two work just fine in 1.0.2 without any ifdefs, I figured I'd upload them now as general cleanup.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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. labels Sep 11, 2017
@BridgeAR
Copy link
Member

@nodejs/crypto PTAL

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 modulo style nit (and a suggestion.)

@@ -1430,10 +1430,13 @@ int SSLWrap<Base>::NewSessionCallback(SSL* s, SSL_SESSION* sess) {
memset(serialized, 0, size);
i2d_SSL_SESSION(sess, &serialized);

unsigned int session_id_length;
const unsigned char *session_id = SSL_SESSION_get_id(sess,
Copy link
Member

Choose a reason for hiding this comment

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

char* (star leans left)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@BridgeAR BridgeAR Sep 21, 2017

Choose a reason for hiding this comment

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

@bnoordhuis is there a way to solve these issues with a new linter? On JS side there are almost no style nits since quite a while but I have seen a lot of those for C++. A issue like this one should be not to difficult to spot for a linter, right?

I do not write C++ normally, so I do not know if there is a good alternative to cpplint but google gave me e.g. http://clang.llvm.org/extra/clang-tidy/ and it seems to be extendable. So we could also add more rules if we are not happy with the PRs.

This should reduce the work for everyone involved as far as I can tell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clang-format is wonderful and quite configurable. There's even a mode where it only applies to the code you touched in your diff. A lot of parts of Chromium require that running through that to upload a change.

Copy link
Member

Choose a reason for hiding this comment

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

Apropos clang-format, see the discussion in #3612.

Copy link
Member

Choose a reason for hiding this comment

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

💯 on having a C++ linter, looking at #3612 it sounds like the massive diff was an issue, but most people were +1 on the idea of having linting.

Given how much we use eslint, and that we've had more and more sweeping changes from it, I think we're probably more open to these kinds of refactorings these days. It could even just start with one rule (like star leans left). Auto fixing issues sounds good too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not just the diff was a problem, clang is hard to get, linux distro packaged versions are out of date, and each version of the formatter seems to do things differently. I think packaging for OS X and Windows was even more problematic than linux. If it was stable, easy to get, widely available, it'd would have flown further.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe packaging it as an npm module might be helpful then.

&p,
ext->value->length,
ASN1_ITEM_ptr(method->it)));
GENERAL_NAMES* names = reinterpret_cast<GENERAL_NAMES*>(X509V3_EXT_d2i(ext));
Copy link
Member

Choose a reason for hiding this comment

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

Since you're here: static_cast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

This accessor exists in OpenSSL 1.0.2, so it may be used already. This
is cherry-picked from PR nodejs#8491.
There is no need to reach into quite so many internals to decode an
extension.
@davidben
Copy link
Contributor Author

The rest of the 1.1.0 PR is taking a bit longer to put together, but it's coming along. My plan is to send you one PR that does the full port, but with all the individual pieces split into separate commits. This way you don't need to worry about landing OPENSSL_VERSION_NUMBER checks without full 1.1.0 support (@shigeki was unhappy with that in a previous PR), but there is some hope for it to be reviewable. I'm also happy to send it in separate PRs if that's preferred.

@BridgeAR
Copy link
Member

@BridgeAR
Copy link
Member

@bnoordhuis would you be so kind and sign off again? I think this could land otherwise.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Sep 28, 2017
This accessor exists in OpenSSL 1.0.2, so it may be used already. This
is cherry-picked from PR nodejs#8491.

PR-URL: nodejs#15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Sep 28, 2017
There is no need to reach into quite so many internals to decode an
extension.

PR-URL: nodejs#15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BridgeAR
Copy link
Member

BridgeAR commented Sep 28, 2017

Landed in c51e0e9 and 9af9bcb

@BridgeAR BridgeAR closed this Sep 28, 2017
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2017
This accessor exists in OpenSSL 1.0.2, so it may be used already. This
is cherry-picked from PR nodejs#8491.

PR-URL: nodejs#15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2017
There is no need to reach into quite so many internals to decode an
extension.

PR-URL: nodejs#15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
This accessor exists in OpenSSL 1.0.2, so it may be used already. This
is cherry-picked from PR #8491.

PR-URL: #15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
There is no need to reach into quite so many internals to decode an
extension.

PR-URL: #15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
This accessor exists in OpenSSL 1.0.2, so it may be used already. This
is cherry-picked from PR #8491.

PR-URL: nodejs/node#15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
There is no need to reach into quite so many internals to decode an
extension.

PR-URL: nodejs/node#15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
This accessor exists in OpenSSL 1.0.2, so it may be used already. This
is cherry-picked from PR #8491.

PR-URL: #15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
There is no need to reach into quite so many internals to decode an
extension.

PR-URL: #15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
This accessor exists in OpenSSL 1.0.2, so it may be used already. This
is cherry-picked from PR #8491.

PR-URL: #15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
There is no need to reach into quite so many internals to decode an
extension.

PR-URL: #15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

@nodejs/crypto this lands cleanly on v6.x and I've opted to cherry-pick it. Please LMK if it should be backed out.

MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
This accessor exists in OpenSSL 1.0.2, so it may be used already. This
is cherry-picked from PR #8491.

PR-URL: #15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
There is no need to reach into quite so many internals to decode an
extension.

PR-URL: #15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
This accessor exists in OpenSSL 1.0.2, so it may be used already. This
is cherry-picked from PR #8491.

PR-URL: #15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
There is no need to reach into quite so many internals to decode an
extension.

PR-URL: #15348
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants