Skip to content

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