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: remove guard against fixed OpenSSL bug #29854

Closed

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Oct 5, 2019

This guard used to prevent segfaults caused by a bug in OpenSSL, but this was fixed in OpenSSL 1.1.1d.

Refs: openssl/openssl#9433
Refs: openssl/openssl#9431

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

This guard used to prevent segfaults caused by a bug in OpenSSL, but
this was fixed in OpenSSL 1.1.1d.

Refs: openssl/openssl#9433
Refs: openssl/openssl#9431
@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 Oct 5, 2019
@richardlau
Copy link
Member

Do we need to update the OpenSSL version in the sharedlib docker containers in the CI to 1.1.1d if this guard is removed?

@tniessen
Copy link
Member Author

tniessen commented Oct 5, 2019

@richardlau I think so.

@sam-github
Copy link
Contributor

Any concerns about distros that might still be linking dynamically against openssl 1.1.1c or below?

My unbuntu is still at 1.1.1b -- though in fairness, its nodejs is 10.x :-( (not that I install node from distro packages).

@bnoordhuis
Copy link
Member

Any concerns about distros that might still be linking dynamically against openssl 1.1.1c or below?

I expect that to be problematic, yes. I would leave it alone for now.

You could wrap it in a guard if you want to ensure it's not forgotten when we upgrade to 1.2.0:

#if OPENSSL_VERSION_NUMBER >= 0x10200000L
#error "Remove this code."
#else
// ...
#endif

@richardlau
Copy link
Member

My unbuntu is still at 1.1.1b -- though in fairness, its nodejs is 10.x :-( (not that I install node from distro packages).

So are our docker containers for the sharedlibs builds: https://github.com/nodejs/build/blob/dc94f7e911ea6d2ea90a28a9cf2966a06f2f12f2/ansible/roles/docker/templates/ubuntu1604_sharedlibs.Dockerfile.j2#L57-L65

If we did update OpenSSL in the docker containers to 1.1.1d we need to have the test fix (3473e58) from #29550 on 12.x to avoid breaking that release line.

@tniessen
Copy link
Member Author

tniessen commented Oct 9, 2019

So I guess there is nothing we can do at this point?

@bnoordhuis
Copy link
Member

Pretty much.

@tniessen
Copy link
Member Author

tniessen commented Oct 9, 2019

I'll reopen this PR once the change becomes possible, that is, after upgrading to 1.2.0 I assume?

@tniessen tniessen closed this Oct 9, 2019
@sam-github
Copy link
Contributor

Not relevant here, but there won't be a 1.2, next release line will be 3.0.0 unless I'm very much mistaken. You could leave a comment saying when it could be deleted, but its maybe not worth the effort.

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