crypto: make some easy parts 1.1.0-compatible #15348

davidben · 2017-09-11T21:48:42Z

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

BridgeAR · 2017-09-19T19:53:10Z

@nodejs/crypto PTAL

bnoordhuis

LGTM modulo style nit (and a suggestion.)

bnoordhuis · 2017-09-20T18:09:47Z

src/node_crypto.cc

@@ -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,


char* (star leans left)

@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.

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.

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

💯 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.

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.

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

bnoordhuis · 2017-09-20T18:11:27Z

src/node_crypto.cc

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


Since you're here: static_cast

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 · 2017-09-20T21:02:22Z

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 · 2017-09-22T22:11:31Z

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

BridgeAR · 2017-09-22T23:31:18Z

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

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>

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 · 2017-09-28T05:12:00Z

Landed in c51e0e9 and 9af9bcb

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>

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>

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>