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: use non-deprecated v8::Object::Set #17482

Closed
wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Dec 6, 2017

This commit updates node_crypto to use the non-deprecated Set functions
that return a v8::Maybe.

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. labels Dec 6, 2017
@danbev
Copy link
Contributor Author

danbev commented Dec 6, 2017

@@ -3562,7 +3600,7 @@ void Connection::SetSNICallback(const FunctionCallbackInfo<Value>& args) {
}

Local<Object> obj = Object::New(env->isolate());
obj->Set(env->onselect_string(), args[0]);
obj->Set(env->context(), env->onselect_string(), args[0]).FromJust();
Copy link
Member

@TimothyGu TimothyGu Dec 6, 2017

Choose a reason for hiding this comment

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

Can exceptions be properly handled rather than crashing directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TimothyGu I'd be happy to do that. In this case would that involve calling ThrowError?

Is this concern specifically to Connection::SetSNICallback or in general for the other changes in this PR as well?

Copy link
Member

Choose a reason for hiding this comment

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

ping @TimothyGu

Copy link
Member

Choose a reason for hiding this comment

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

@danbev To give a bit of context: An empty Maybe(Local) in the V8 API always means that there is already a pending exception, i.e. there’s no ThrowError necessary on our side.

For example, this Set() would return an empty Maybe if there is a setter for onselect and that throws an exception; I think in this situation that can only happen if somebody defined one on Object.prototype itself.

In this case: Since the function is called directly from JS, and there’s no cleanup of any sort necessary, it should be fine to just return; here if the Maybe is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax Thanks for the details on this, I appreciate that!

Just to verify my understanding here.... Taking the example you suggested, if there is a setter for onselect:

Object.defineProperty(Object.prototype, 'onselect', {
  set: function(f) {
    console.log('throw error from setter...');
    throw Error('dummy setter error');
  }
});

And setSNICallback is called, with the current solution the exception reported would be:

$ out/Debug/node  test/parallel/test-tls-legacy-onselect.js
throw error from setter...
FATAL ERROR: v8::FromJust Maybe value is Nothing.
 1: node::Abort() [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
 2: node::OnFatalError(char const*, char const*) [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
 3: v8::Utils::ReportApiFailure(char const*, char const*) [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
 4: v8::Utils::ApiCheck(bool, char const*, char const*) [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
 5: v8::V8::FromJustIsNothing() [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
 6: v8::Maybe<bool>::FromJust() const [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
 7: node::crypto::Connection::SetSNICallback(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
 8: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
 9: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
10: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
11: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [/Users/danielbevenius/work/nodejs/node/out/Debug/node]
12: 0x10e0737043c4
13: 0x10e0737f1600
Abort trap: 6

But instead what should happen is (in SetSNICallback):

if (obj->Set(env->context(), env->onselect_string(), args[0]).IsJust()) {
    conn->sniObject_.Reset(args.GetIsolate(), obj);
}

And the reported error would be:

throw error from setter...
/Users/danielbevenius/work/nodejs/node/test/parallel/test-tls-legacy-onselect.js:13
    throw Error('dummy setter error');
    ^

Error: dummy setter error
    at Object.set (/Users/danielbevenius/work/nodejs/node/test/parallel/test-tls-legacy-onselect.js:13:11)
    at Server.<anonymous> (/Users/danielbevenius/work/nodejs/node/test/parallel/test-tls-legacy-onselect.js:20:12)
    at Server.<anonymous> (/Users/danielbevenius/work/nodejs/node/test/common/index.js:520:15)
    at Server.emit (events.js:126:13)
    at TCP.onconnection (net.js:1595:8)

Copy link
Member

Choose a reason for hiding this comment

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

@danbev Yup, that’s what I’m suggesting (and I think @TimothyGu as well). 👍

Just as a note, I think it’s more common (and more readable) to return early rather than having the Reset() call inside the conditional, even if it boils down to the same thing.

Copy link
Member

Choose a reason for hiding this comment

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

Apropos this particular line, if the Set() call fails, something is probably very wrong. Handle failures when there is a reasonable expectation an operation can fail and otherwise let it crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnoordhuis I just want to make sure I'm not misunderstanding you here. Do you mean that in this case we should call FromJust()?

@@ -2385,7 +2404,8 @@ void SSLWrap<Base>::VerifyError(const FunctionCallbackInfo<Value>& args) {
Local<String> reason_string = OneByteString(isolate, reason);
Local<Value> exception_value = Exception::Error(reason_string);
Local<Object> exception_object = exception_value->ToObject(isolate);
exception_object->Set(w->env()->code_string(), OneByteString(isolate, code));
exception_object->Set(env->context(), w->env()->code_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Could use w->env()->context() instead.

target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "Connection"),
t->GetFunction());
target->Set(env->context(), FIXED_ONE_BYTE_STRING(env->isolate(),
"Connection"),
Copy link
Member

Choose a reason for hiding this comment

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

Awkward line break. Can you put the FIXED_ONE_BYTE_STRING on a new line?

@@ -3562,7 +3600,7 @@ void Connection::SetSNICallback(const FunctionCallbackInfo<Value>& args) {
}

Local<Object> obj = Object::New(env->isolate());
obj->Set(env->onselect_string(), args[0]);
obj->Set(env->context(), env->onselect_string(), args[0]).FromJust();
Copy link
Member

Choose a reason for hiding this comment

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

Apropos this particular line, if the Set() call fails, something is probably very wrong. Handle failures when there is a reasonable expectation an operation can fail and otherwise let it crash.

@BridgeAR
Copy link
Member

BridgeAR commented Jan 19, 2018

This seems stalled. @danbev where do we stand here?

@BridgeAR BridgeAR added the stalled Issues and PRs that are stalled. label Jan 19, 2018
@danbev
Copy link
Contributor Author

danbev commented Jan 20, 2018

@BridgeAR I hope to revisit this next week (though I've got a sick kid here and I might not be working at all next week by the looks of it)

@BridgeAR
Copy link
Member

Ping @danbev

This commit updates node_crypto to use the non-deprecated Set functions
that return a v8::Maybe<bool>.
@danbev
Copy link
Contributor Author

danbev commented Jan 31, 2018

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed stalled Issues and PRs that are stalled. labels Jan 31, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

Landed in ff21fb1

@BridgeAR BridgeAR closed this Feb 1, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 1, 2018
This commit updates node_crypto to use the non-deprecated Set functions
that return a v8::Maybe<bool>.

PR-URL: nodejs#17482
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
This commit updates node_crypto to use the non-deprecated Set functions
that return a v8::Maybe<bool>.

PR-URL: #17482
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
This commit updates node_crypto to use the non-deprecated Set functions
that return a v8::Maybe<bool>.

PR-URL: #17482
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
This commit updates node_crypto to use the non-deprecated Set functions
that return a v8::Maybe<bool>.

PR-URL: #17482
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
@danbev danbev deleted the crypto_set_with_context branch February 28, 2018 07:30
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging or v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This commit updates node_crypto to use the non-deprecated Set functions
that return a v8::Maybe<bool>.

PR-URL: nodejs#17482
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins
Copy link
Contributor

ping re: backport @danbev

@danbev
Copy link
Contributor Author

danbev commented May 23, 2018

@MylesBorins Sorry I missed this 😞 I'll take a look at backporting tomorrow.

danbev added a commit to danbev/node that referenced this pull request May 24, 2018
This commit updates node_crypto to use the non-deprecated Set functions
that return a v8::Maybe<bool>.

PR-URL: nodejs#17482
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
This commit updates node_crypto to use the non-deprecated Set functions
that return a v8::Maybe<bool>.

Backport-PR-URL: #20931
PR-URL: #17482
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
This commit updates node_crypto to use the non-deprecated Set functions
that return a v8::Maybe<bool>.

Backport-PR-URL: #20931
PR-URL: #17482
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

10 participants