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.

Sign up for GitHub

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
Closed

crypto: don't reference |freelist_max_len| in OpenSSL 1.1.0. #10859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/_tls_common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ exports.createSecureContext = function createSecureContext(options, context) {
}
}

// Do not keep read/write buffers in free list
// Do not keep read/write buffers in free list for OpenSSL < 1.1.0. (For
// OpenSSL 1.1.0, buffers are malloced and freed without the use of a
// freelist.)
if (options.singleUse) {
c.singleUse = true;
c.context.setFreeListLength(0);
Expand Down
4 changes: 4 additions & 0 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

// |freelist_max_len| was removed in OpenSSL 1.1.0. In that version OpenSSL
// mallocs and frees buffers directly, without the use of a freelist.
SecureContext* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());

wrap->ctx_->freelist_max_len = args[0]->Int32Value();
#endif
}


Expand Down