src: add HAVE_OPENSSL guard to crypto providers#12967

danbev · 2017-05-11T08:19:54Z

When configured --without-ssl node_crypto.h will not be included but
async-wrap.h includes providers that are defined in node_crypto.h,
node_crypto.cc, and tls_wrap.cc:
AsyncWrap::PROVIDER_CONNECTION
AsyncWrap::PROVIDER_PBKDF2REQUEST
AsyncWrap::PROVIDER_RANDOMBYTESREQUEST
AsyncWrap::PROVIDER_TLSWRAP

These will be included as providers which will cause
test-async-wrap-getasyncid.js to fail.

This commit suggest adding a guard and exclude the providers that are
not available when configured --without-ssl

Checklist

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

Affected core subsystem(s)

src

When configured --without-ssl node_crypto.h will not be included but async-wrap.h includes providers that are defined in node_crypto.h, node_crypto.cc, and tls_wrap.cc: AsyncWrap::PROVIDER_CONNECTION AsyncWrap::PROVIDER_PBKDF2REQUEST AsyncWrap::PROVIDER_RANDOMBYTESREQUEST AsyncWrap::PROVIDER_TLSWRAP These will be included as providers which will cause test-async-wrap-getasyncid.js to fail. This commit suggest adding a guard and exclude the providers that are not available when configured --without-ssl

danbev · 2017-05-11T08:21:59Z

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

gibfahn · 2017-05-11T11:42:31Z

src/async-wrap.h

 #defineNODE_ASYNC_ID_OFFSET0xA1C

-#defineNODE_ASYNC_PROVIDER_TYPES(V) \
+#defineNODE_ASYNC_NONE_CRYPTO_PROVIDER_TYPES(V) \


Nit: could we use NO_CRYPTO instead of NONE_CRYPTO?

Good point, I'll change that. Thanks

gibfahn

LGTM, although I'd like someone with more experience here to confirm.

gibfahn · 2017-05-11T11:46:25Z

just-in-case cc/ @trevnorris @bnoordhuis (although I think they're both pretty busy recently).

danbev · 2017-05-11T11:47:43Z

I'm running the test on windows locally to see if I can reproduce the windows-fanned error reported which does look related:

ok 1 parallel/test-assert-fail --- duration_ms: 0.145 ...not ok 2 parallel/test-async-wrap-getasyncid --- duration_ms: 0.262 severity: fail stack: |- Mismatched <anonymous> function calls. Expected 1, actual 0. at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win2008r2\test\parallel\test-async-wrap-getasyncid.js:155:42) at Module._compile (module.js:569:30) at Object.Module._extensions..js (module.js:580:10) at Module.load (module.js:503:32) at tryModuleLoad (module.js:466:12) at Function.Module._load (module.js:458:3) at Function.Module.runMain (module.js:605:10) at startup (bootstrap_node.js:144:16) at bootstrap_node.js:561:3 (node:2036) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.Socket instead.

The test/osx I'm not sure about:

not ok 589 parallel/test-http-res-write-after-end --- duration_ms: 7.7 severity: crashed stack: |- oh no! exit code: CRASHED (Signal: 9) ...not ok 590 parallel/test-http-request-methods --- duration_ms: 7.61 severity: crashed stack: |- oh no! exit code: CRASHED (Signal: 9)

I don't see this locally but lately I do get test failures now and again.

addaleax · 2017-05-11T12:42:21Z

src/async-wrap.h


+#if HAVE_OPENSSL
+#defineNODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) \
+V(CONNECTION) \


Maybe not in this PR, but we should probably rename this to SSLCONNECTION or something like that?

gibfahn · 2017-05-11T13:24:26Z

cc/ @nodejs/platform-windows for the above CI failure

refack · 2017-05-11T18:49:37Z

Looking into it, did the test fail with or without ssl?

refack · 2017-05-11T18:55:03Z

src/async-wrap.h

 #defineNODE_ASYNC_ID_OFFSET0xA1C

-#defineNODE_ASYNC_PROVIDER_TYPES(V) \
+#defineNODE_ASYNC_NO_CRYPTO_PROVIDER_TYPES(V) \


Shouldn't it be ASYNC_NON_CRYPTO?

~~It's for when you build with no crypto, so for me NO_CRYPTO sounds better.~~
I retract that, these are the non-crypto provider types aren't they, in which case I think you're right, NON_CRYPTO makes more sense.
sorry for the churn @danbev!

No problems, I'll update.

refack · 2017-05-11T19:41:34Z

No repro when built with SSL.

refack · 2017-05-11T20:11:49Z

No repro when built without SSL.
Difference is I'm building with VS2017.
Now trying with 2015.
But I think the test is flaky.

refack · 2017-05-11T20:18:36Z

Stress: https://ci.nodejs.org/job/node-stress-single-test/1206/nodes=win2012r2/

refack · 2017-05-11T20:40:04Z

I've rerun CI: https://ci.nodejs.org/job/node-test-commit/9820/

refack · 2017-05-11T21:25:45Z

So now it's green, the freeBSD are flakes #12950 & #12951

refack

Name churn, sorry...

danbev · 2017-05-12T03:51:52Z

Name churn, sorry...

No problems, I'll update this.

danbev · 2017-05-12T04:06:41Z

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

When configured --without-ssl node_crypto.h will not be included but async-wrap.h includes providers that are defined in node_crypto.h, node_crypto.cc, and tls_wrap.cc: AsyncWrap::PROVIDER_CONNECTION AsyncWrap::PROVIDER_PBKDF2REQUEST AsyncWrap::PROVIDER_RANDOMBYTESREQUEST AsyncWrap::PROVIDER_TLSWRAP These will be included as providers which will cause test-async-wrap-getasyncid.js to fail. This commit suggest adding a guard and exclude the providers that are not available when configured --without-ssl PR-URL: nodejs#12967 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

danbev · 2017-05-12T06:11:26Z

Landed in 1541079

Currently the async provider type CONNECTION is used in node_crypto.h and it might be clearer if it was named SSLCONNECTION as suggested by addaleax. This commit renames only the provider type as I was not sure if it was alright to change the class Connection as well. Refs: nodejs#12967 (comment)

gibfahn · 2017-05-12T09:24:40Z

@danbev generally good to leave PRs open for at least 48/72 hours to make sure everyone gets a chance to see and review them (unless there's a reason to land sooner). No big deal in this case, just a reminder 😄 .

danbev · 2017-05-12T09:43:38Z

@gibfahn Sorry about that, for some reason I though this had been open 48 hour, but it has only been open half that time 😞 . Thanks for the heads up and I'll be more careful in the future.

refack · 2017-05-12T13:24:19Z

If anyone sees parallel/test-async-wrap-getasyncid flake again please ping me...

Currently the async provider type CONNECTION is used in node_crypto.h and it might be clearer if it was named SSLCONNECTION as suggested by addaleax. This commit renames only the provider type as I was not sure if it was alright to change the class Connection as well. Refs: nodejs#12967 (comment) PR-URL: nodejs#12989 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

When configured --without-ssl node_crypto.h will not be included but async-wrap.h includes providers that are defined in node_crypto.h, node_crypto.cc, and tls_wrap.cc: AsyncWrap::PROVIDER_CONNECTION AsyncWrap::PROVIDER_PBKDF2REQUEST AsyncWrap::PROVIDER_RANDOMBYTESREQUEST AsyncWrap::PROVIDER_TLSWRAP These will be included as providers which will cause test-async-wrap-getasyncid.js to fail. This commit suggest adding a guard and exclude the providers that are not available when configured --without-ssl PR-URL: nodejs#12967 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Currently the async provider type CONNECTION is used in node_crypto.h and it might be clearer if it was named SSLCONNECTION as suggested by addaleax. This commit renames only the provider type as I was not sure if it was alright to change the class Connection as well. Refs: nodejs#12967 (comment) PR-URL: nodejs#12989 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

MylesBorins · 2017-06-22T18:14:37Z

I'm assuming this doesn't make sense for v6.x unless we backport all of async_hooks... please add don't land label if appropriate

nodejs-github-bot added async_wrap c++ Issues and PRs that require attention from people who are familiar with C++. labels May 11, 2017

gibfahn reviewed May 11, 2017
View reviewed changes

gibfahn approved these changes May 11, 2017
View reviewed changes

change to NODE_ASYNC_NO_CRYPTO_PROVIDER_TYPES
81d8720

addaleax reviewed May 11, 2017
View reviewed changes

addaleax approved these changes May 11, 2017
View reviewed changes

sam-github approved these changes May 11, 2017
View reviewed changes

cjihrig approved these changes May 11, 2017
View reviewed changes

refack reviewed May 11, 2017
View reviewed changes

refack suggested changes May 12, 2017
View reviewed changes

change to ASYNC_NON_CRYPTO_PROVIDER
05f851a

refack approved these changes May 12, 2017
View reviewed changes

danbev closed this May 12, 2017

danbev deleted the asyncwrap-have-openssl-guard branch May 12, 2017 06:11

danbev mentioned this pull request May 12, 2017
src: rename CONNECTION provider to SSLCONNECTION #12989
Closed
2 tasks

jasnell mentioned this pull request May 28, 2017
8.0.0 Release Proposal #12220
Closed

gibfahn mentioned this pull request Jun 15, 2017
Auditing for 6.11.1 nodejs/Release#230
Closed
3 tasks

MylesBorins added the lts-watch-v6.x label Jun 22, 2017

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Aug 14, 2017

Uh oh!

src: add HAVE_OPENSSL guard to crypto providers#12967

src: add HAVE_OPENSSL guard to crypto providers #12967

Uh oh!

Conversation

danbev commented May 11, 2017

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented May 11, 2017

Uh oh!

gibfahnMay 11, 2017

Choose a reason for hiding this comment

Uh oh!

danbevMay 11, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

gibfahn commented May 11, 2017

Uh oh!

danbev commented May 11, 2017

Uh oh!

addaleaxMay 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented May 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented May 11, 2017

Uh oh!

refackMay 11, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahnMay 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbevMay 12, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented May 11, 2017

Uh oh!

refack commented May 11, 2017

Uh oh!

refack commented May 11, 2017

Uh oh!

refack commented May 11, 2017

Uh oh!

refack commented May 11, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

danbev commented May 12, 2017

Uh oh!

danbev commented May 12, 2017

Uh oh!

danbev commented May 12, 2017

Uh oh!

gibfahn commented May 12, 2017

Uh oh!

danbev commented May 12, 2017

Uh oh!

refack commented May 12, 2017

Uh oh!

MylesBorins commented Jun 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

addaleaxMay 11, 2017•
edited
Loading

gibfahn commented May 11, 2017•
edited
Loading

gibfahnMay 11, 2017•
edited
Loading