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

src: add HAVE_OPENSSL guard to crypto providers #12967

Closed
wants to merge 3 commits into from

Conversation

@danbev
Copy link
Member

commented May 11, 2017

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

src: add HAVE_OPENSSL guard to crypto providers
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

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

@@ -34,34 +34,44 @@ namespace node {

#define NODE_ASYNC_ID_OFFSET 0xA1C

#define NODE_ASYNC_PROVIDER_TYPES(V) \
#define NODE_ASYNC_NONE_CRYPTO_PROVIDER_TYPES(V) \

This comment has been minimized.

Copy link
@gibfahn

gibfahn May 11, 2017

Member

Nit: could we use NO_CRYPTO instead of NONE_CRYPTO?

This comment has been minimized.

Copy link
@danbev

danbev May 11, 2017

Author Member

Good point, I'll change that. Thanks

@gibfahn
Copy link
Member

left a comment

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

@gibfahn

This comment has been minimized.

Copy link
Member

commented May 11, 2017

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

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

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.

V(TTYWRAP) \
V(UDPSENDWRAP) \
V(UDPWRAP) \
V(WRITEWRAP) \
V(ZLIB)

#if HAVE_OPENSSL
#define NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) \
V(CONNECTION) \

This comment has been minimized.

Copy link
@addaleax

addaleax May 11, 2017

Member

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

@gibfahn

This comment has been minimized.

Copy link
Member

commented May 11, 2017

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

@refack

This comment has been minimized.

Copy link
Member

commented May 11, 2017

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

@@ -34,34 +34,44 @@ namespace node {

#define NODE_ASYNC_ID_OFFSET 0xA1C

#define NODE_ASYNC_PROVIDER_TYPES(V) \
#define NODE_ASYNC_NO_CRYPTO_PROVIDER_TYPES(V) \

This comment has been minimized.

Copy link
@refack

refack May 11, 2017

Member

Shouldn't it be ASYNC_NON_CRYPTO?

This comment has been minimized.

Copy link
@gibfahn

gibfahn May 11, 2017

Member

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!

This comment has been minimized.

Copy link
@danbev

danbev May 12, 2017

Author Member

No problems, I'll update.

@refack

This comment has been minimized.

Copy link
Member

commented May 11, 2017

No repro when built with SSL.

@refack

This comment has been minimized.

Copy link
Member

commented May 11, 2017

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

This comment has been minimized.

@refack

This comment has been minimized.

Copy link
Member

commented May 11, 2017

@refack

This comment has been minimized.

Copy link
Member

commented May 11, 2017

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

@refack
Copy link
Member

left a comment

Name churn, sorry...

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 12, 2017

Name churn, sorry...

No problems, I'll update this.

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 12, 2017

@refack
refack approved these changes May 12, 2017
danbev added a commit to danbev/node that referenced this pull request May 12, 2017
src: add HAVE_OPENSSL guard to crypto providers
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 <gibfahn@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@danbev

This comment has been minimized.

Copy link
Member Author

commented May 12, 2017

Landed in 1541079

@danbev danbev closed this May 12, 2017

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

danbev added a commit to danbev/node that referenced this pull request May 12, 2017
src: rename CONNECTION provider to SSLCONNECTION
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)
@danbev danbev referenced this pull request May 12, 2017
2 of 2 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented May 12, 2017

@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

This comment has been minimized.

Copy link
Member Author

commented May 12, 2017

@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

This comment has been minimized.

Copy link
Member

commented May 12, 2017

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

danbev added a commit to danbev/node that referenced this pull request May 15, 2017
src: rename CONNECTION provider to SSLCONNECTION
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 <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
src: add HAVE_OPENSSL guard to crypto providers
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 <gibfahn@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
src: rename CONNECTION provider to SSLCONNECTION
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 <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 22, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.