process,tls: stop relying on process.features #21087
Closed
Conversation
This file contains hidden or 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
The `process.features` property is mutable by userland such that
`delete process.features` will cause SNI and ALPN support for fail.
Add the necessary properties to `process.config('binding')` so that
we are not relying on a user-modifiable object.
Also clean up the `GetFeatures` funciton just a bit.
jasnell
added
tls
addaleax
approved these changes
Jun 1, 2018
src/node.cc
Outdated
| #else | ||
| Local<Boolean> tls_ocsp = False(env->isolate()); | ||
| Local<Boolean> tls_ocsp = falseValue; |
There was a problem hiding this comment.
style nit: true_value and false_value
devsnek
approved these changes
Jun 1, 2018
mscdex
reviewed
Jun 2, 2018
| EscapableHandleScope scope(env->isolate()); | ||
| Isolate* isolate = env->isolate(); | ||
| EscapableHandleScope scope(isolate); | ||
| Local<Boolean> true_value = True(isolate); |
There was a problem hiding this comment.
Why do we need these two separate variables instead of using True(isolate) and False(isolate) inline like before?
mscdex
reviewed
Jun 2, 2018
| #else | ||
| Local<Value> debug = False(env->isolate()); | ||
| Local<Boolean> debug = false_value; |
There was a problem hiding this comment.
Instead of making this change multiple times, what if we instead removed the temporary variables instead and just set the values directly in the obj->Set() calls? For example:
obj->Set(FIXED_ONE_BYTE_STRING(isolate, "debug"),
#if defined(DEBUG) && DEBUG
True(isolate)
#else
False(isolate)
#endif
);or something similar or maybe even write a macro for this?
|
In the words of the great poet Zappa: I've got a better idea! See #21094, we can get rid of the conditionals altogether, they're remnants from when we supported older versions of openssl. |
|
Even better. |
|
Closing in favor of #21094! |
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.
The
process.featuresproperty is mutable by userland such thatdelete process.featureswill cause SNI and ALPN support for fail.Add the necessary properties to
process.config('binding')so thatwe are not relying on a user-modifiable object.
Also clean up the
GetFeaturesfunciton just a bit.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes