build: fix compiling against openssl with no-psk #36881
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
the
c++
|
afaict node doesn't have integration testing for different openssl configurations. I've tested this change using the script from #36464, it compiles and all except two psk tests pass. in both tests the error is |
jasnell
approved these changes
Jan 11, 2021
github-actions
bot
removed
the
request-ci
|
I don't understand how this change would introduce a memory leak. is test-asan flaky? |
It's not flaky, it was broken by #36779. |
Hmm... I missed that entirely. I'll take a look |
aduh95
pushed a commit
to jasnell/node
that referenced
this pull request
Jan 12, 2021
The asan checks don't play well currently with persistent secure heap allocations. Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: nodejs#36900 Refs: nodejs#36881 Reviewed-By: Danielle Adams <adamzdanielle@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
|
if asan is already broken are there any blockers of this PR? |
danielleadams
pushed a commit
that referenced
this pull request
Jan 13, 2021
The asan checks don't play well currently with persistent secure heap allocations. Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #36900 Refs: #36881 Reviewed-By: Danielle Adams <adamzdanielle@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
fixes nodejs#36464 Node 15 prior to this commit will not compile if openssl is built with no-psk. Compiling emits an error like this: ``` crypto_tls.cc:(.text+0x4c27): undefined reference to `node::crypto::TLSWrap::SetCACerts(node::crypto::SecureContext*)' ``` Blame on crypto_tls.cc shows the file was created in a refactor. Before that refactor SetCACerts was defined outside OPENSSL_NO_PSK ifndef https://github.com/nodejs/node/blob/6751b6dc3da102f259b74b7453032edadc7a37ca/src/crypto/crypto_ssl.cc#L839
aduh95
added
the
author ready
richardlau
approved these changes
Jan 18, 2021
|
@everett1992 GitHub cannot link the commit to your GitHub account. You may want to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork to set this up – so you'll get a |
|
@aduh95 thanks for letting me know, I'm okay not being marked as a contributor |
aduh95
pushed a commit
that referenced
this pull request
Jan 19, 2021
Node 15 prior to this commit will not compile if openssl is built with no-psk. Compiling emits an error like this: ``` crypto_tls.cc:(.text+0x4c27): undefined reference to `node::crypto::TLSWrap::SetCACerts(node::crypto::SecureContext*)' ``` Blame on crypto_tls.cc shows the file was created in a refactor. Before that refactor SetCACerts was defined outside OPENSSL_NO_PSK ifndef. PR-URL: #36881 Fixes: #36464 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
|
Landed in fef2128 |
ruyadorno
pushed a commit
that referenced
this pull request
Jan 22, 2021
Node 15 prior to this commit will not compile if openssl is built with no-psk. Compiling emits an error like this: ``` crypto_tls.cc:(.text+0x4c27): undefined reference to `node::crypto::TLSWrap::SetCACerts(node::crypto::SecureContext*)' ``` Blame on crypto_tls.cc shows the file was created in a refactor. Before that refactor SetCACerts was defined outside OPENSSL_NO_PSK ifndef. PR-URL: #36881 Fixes: #36464 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Merged
ruyadorno
pushed a commit
that referenced
this pull request
Jan 25, 2021
Node 15 prior to this commit will not compile if openssl is built with no-psk. Compiling emits an error like this: ``` crypto_tls.cc:(.text+0x4c27): undefined reference to `node::crypto::TLSWrap::SetCACerts(node::crypto::SecureContext*)' ``` Blame on crypto_tls.cc shows the file was created in a refactor. Before that refactor SetCACerts was defined outside OPENSSL_NO_PSK ifndef. PR-URL: #36881 Fixes: #36464 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #36464
Node 15 prior to this commit will not compile if openssl is built with
no-psk. Compiling emits an error like this:
Blame on crypto_tls.cc shows the file was created in a refactor. Before
that refactor SetCACerts was defined outside OPENSSL_NO_PSK ifndef
node/src/crypto/crypto_ssl.cc
Line 839 in 6751b6d