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: don't reference |freelist_max_len| in OpenSSL 1.1.0. #10859

Closed
wants to merge 1 commit into from

Conversation

agl
Copy link
Contributor

@agl agl commented Jan 17, 2017

The |freelist_max_len| (and the freelist itself) has been removed in
OpenSSL 1.1.0. Thus this change will be necessary at some point but,
for now, it makes it a little easier to build with 1.1.0 without
breaking anything for previous versions of OpenSSL.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

@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. tls Issues and PRs related to the tls subsystem. lts-watch-v6.x labels Jan 17, 2017
@@ -1147,10 +1147,14 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) {


void SecureContext::SetFreeListLength(const FunctionCallbackInfo<Value>& args) {
#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this, but another approach would be to simply not define .setFreeListLength() for ossl versions that don't support it. Its not a public API, we don't have to keep it, and then the code in _tls_common.js would just not call the API if it doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to do this if people prefer, although you would have to tell me what the recommended pattern in Javascript is for testing whether a function exists or not.

Copy link
Contributor

@sam-github sam-github Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets get one more person to weigh in. My pitch is that it is the first step towards just removing the function, which we will want to do, why keep a function around that does nothing? Its very confusing! Also, it makes feature testing very easy.

But its a waste of your time to refactor like I suggest unless @nodejs/crypto or someone else agrees. Lets listen.

You would basically take the ifdef you have around the fn internals now and move them to https://github.com/agl/node/blob/c6404a264e5587dedd0a495f1c91e6d90edcc7ac/src/node_crypto.cc#L297 so the js binding does not get defined. If there are no other uses of SetFreeListLength, you'd have to ifdef out its prototype and def'n as well to avoid compiler whinging.

pattern for testing whether function exists before use is just

if(c.context.setFreeListLength)
  c.context.setFreeListLength(0);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current approach is fine. Sam's suggestion would make the diff quite a bit larger.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, current approach is better. We can clean up all at once later.

@sam-github
Copy link
Contributor

Commit message shouldn't have a ., and should be <= 50 chars. You can remove the test checkbox from the PR description, and the docs, it needs neither.

@agl
Copy link
Contributor Author

agl commented Jan 17, 2017

Commit updated so that the first line is <= 50 chars.

@mscdex
Copy link
Contributor

mscdex commented Jan 17, 2017

Minor nit: I think the pipes can be dropped in the commit message.

The freelist_max_len member of SSL* (and the freelist itself) has been
removed in OpenSSL 1.1.0. Thus this change will be necessary at some
point but, for now, it makes it a little easier to build with 1.1.0
without breaking anything for previous versions of OpenSSL.
@@ -1147,10 +1147,14 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) {


void SecureContext::SetFreeListLength(const FunctionCallbackInfo<Value>& args) {
#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current approach is fine. Sam's suggestion would make the diff quite a bit larger.

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -1147,10 +1147,14 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) {


void SecureContext::SetFreeListLength(const FunctionCallbackInfo<Value>& args) {
#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, current approach is better. We can clean up all at once later.

@sam-github
Copy link
Contributor

Sam's suggestion would make the diff quite a bit larger.

Isn't bigger better? 💪

OK, I'm good with approach as-is, LGTM (still)

@shigeki
Copy link
Contributor

shigeki commented Jan 19, 2017

How about disabling freelist entirely? It gives more simplicity and compatibility between 1.0.2 and 1.1.0.

diff --git a/deps/openssl/openssl.gypi b/deps/openssl/openssl.gypi
index 871cec0..e260cb5 100644
--- a/deps/openssl/openssl.gypi
+++ b/deps/openssl/openssl.gypi
@@ -1268,6 +1268,9 @@
       # the real driver but that poses a security liability when an attacker
       # is able to create a malicious DLL in one of the default search paths.
       'OPENSSL_NO_HW',
+
+      # Disable freelist for it has no longer benefits
+      'OPENSSL_NO_BUF_FREELISTS'
     ],
     'openssl_default_defines_win': [
       'MK1MF_BUILD',
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index f843667..af2d729 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -1150,7 +1150,7 @@ void SecureContext::SetFreeListLength(const FunctionCallbackInfo
   SecureContext* wrap;
   ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());

-  wrap->ctx_->freelist_max_len = args[0]->Int32Value();
+  // Do nothing. To be deprecated in the future.
 }

@bnoordhuis
Copy link
Member

@shigeki I had the same thought but that would reintroduce the memory consumption issue in builds that link against a shared openssl. If we feel that's acceptable, then by all means go for it.

@sam-github
Copy link
Contributor

I read through #1522 and I don't see the connection to "shared ssl", @bnoordhuis, what is different about shared SSL vs ours?

@bnoordhuis
Copy link
Member

@sam-github The system openssl won't be compiled with -DOPENSSL_NO_BUF_FREELISTS so if we don't zero freelist_max_len, it's going to use (lots) more memory.

@sam-github
Copy link
Contributor

I don't think we should break user's of system SSL, it seems unhelpful to linux distros, with not much gain to us. Once we move to new OpenSSL versions, this code seems like it will naturally go away, so long term, we won't have to maintain it.

@shigeki
Copy link
Contributor

shigeki commented Jan 23, 2017

I missed thinking of shared openssl. It means we cannot change and fix ssl behaviors by using build defines or opensslconf.h unfortunately.
This PR has enough approvals so I will land this now.

@shigeki
Copy link
Contributor

shigeki commented Jan 23, 2017

@shigeki
Copy link
Contributor

shigeki commented Jan 23, 2017

CI is green. Landed in a57e2f2. Thanks.

@shigeki shigeki closed this Jan 23, 2017
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
The freelist_max_len member of SSL* (and the freelist itself) has been
removed in OpenSSL 1.1.0. Thus this change will be necessary at some
point but, for now, it makes it a little easier to build with 1.1.0
without breaking anything for previous versions of OpenSSL.

PR-URL: nodejs#10859
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
The freelist_max_len member of SSL* (and the freelist itself) has been
removed in OpenSSL 1.1.0. Thus this change will be necessary at some
point but, for now, it makes it a little easier to build with 1.1.0
without breaking anything for previous versions of OpenSSL.

PR-URL: nodejs#10859
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
@MylesBorins
Copy link
Contributor

I've gone ahead and landed this on v4.x and v6.x staging... the test suites are passing.

if for any reason this should be reverted please @ me

MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
The freelist_max_len member of SSL* (and the freelist itself) has been
removed in OpenSSL 1.1.0. Thus this change will be necessary at some
point but, for now, it makes it a little easier to build with 1.1.0
without breaking anything for previous versions of OpenSSL.

PR-URL: #10859
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
The freelist_max_len member of SSL* (and the freelist itself) has been
removed in OpenSSL 1.1.0. Thus this change will be necessary at some
point but, for now, it makes it a little easier to build with 1.1.0
without breaking anything for previous versions of OpenSSL.

PR-URL: #10859
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
@sam-github
Copy link
Contributor

I think backporting is right, @nodejs/crypto any of you disagree?

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
The freelist_max_len member of SSL* (and the freelist itself) has been
removed in OpenSSL 1.1.0. Thus this change will be necessary at some
point but, for now, it makes it a little easier to build with 1.1.0
without breaking anything for previous versions of OpenSSL.

PR-URL: #10859
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
The freelist_max_len member of SSL* (and the freelist itself) has been
removed in OpenSSL 1.1.0. Thus this change will be necessary at some
point but, for now, it makes it a little easier to build with 1.1.0
without breaking anything for previous versions of OpenSSL.

PR-URL: #10859
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
@shigeki
Copy link
Contributor

shigeki commented Mar 9, 2017

I agree this. It gives a code consistency.

bbondy pushed a commit to brave/node that referenced this pull request Apr 9, 2017
The freelist_max_len member of SSL* (and the freelist itself) has been
removed in OpenSSL 1.1.0. Thus this change will be necessary at some
point but, for now, it makes it a little easier to build with 1.1.0
without breaking anything for previous versions of OpenSSL.

PR-URL: nodejs/node#10859
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
@rvagg rvagg mentioned this pull request Nov 10, 2017
3 tasks
tniessen added a commit to tniessen/node that referenced this pull request Aug 20, 2022
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: nodejs#1529
Refs: nodejs#10859
Refs: nodejs#19794
Refs: nodejs#38116
nodejs-github-bot pushed a commit that referenced this pull request Aug 22, 2022
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: #1529
Refs: #10859
Refs: #19794
Refs: #38116
PR-URL: #44300
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
RafaelGSS pushed a commit that referenced this pull request Sep 5, 2022
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: #1529
Refs: #10859
Refs: #19794
Refs: #38116
PR-URL: #44300
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: nodejs#1529
Refs: nodejs#10859
Refs: nodejs#19794
Refs: nodejs#38116
PR-URL: nodejs#44300
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this pull request Oct 3, 2022
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: #1529
Refs: #10859
Refs: #19794
Refs: #38116
PR-URL: #44300
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: #1529
Refs: #10859
Refs: #19794
Refs: #38116
PR-URL: #44300
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this pull request Oct 7, 2022
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: #1529
Refs: #10859
Refs: #19794
Refs: #38116
PR-URL: #44300
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this pull request Oct 10, 2022
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: #1529
Refs: #10859
Refs: #19794
Refs: #38116
PR-URL: #44300
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this pull request Oct 11, 2022
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: #1529
Refs: #10859
Refs: #19794
Refs: #38116
PR-URL: #44300
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: nodejs/node#1529
Refs: nodejs/node#10859
Refs: nodejs/node#19794
Refs: nodejs/node#38116
PR-URL: nodejs/node#44300
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
This function was introduced in 2684c90
as an internal helper function. The C++ implementation became a no-op in
a57e2f2 when building against OpenSSL
1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all
supported OpenSSL versions in 970ce14.
Finally, eb20447 removed the only call
site of setFreeListLength (which was already a no-op at that point).

Refs: nodejs/node#1529
Refs: nodejs/node#10859
Refs: nodejs/node#19794
Refs: nodejs/node#38116
PR-URL: nodejs/node#44300
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants