crypto: use non-deprecated v8::Object::Set

[danbev]

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

[danbev]

CI: https://ci.nodejs.org/job/node-test-pull-request/11906/

[TimothyGu]

Can exceptions be properly handled rather than crashing directly?

[danbev]

@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?

[apapirovski]

ping @TimothyGu

[addaleax]

@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.

[danbev]

@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)

[addaleax]

@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.

[bnoordhuis]

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.

[danbev]

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

[bnoordhuis]

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

[bnoordhuis]

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

[bnoordhuis]

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]

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

[danbev]

@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]

Ping @danbev

[danbev]

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/12835/

[BridgeAR]

Landed in ff21fb1

[MylesBorins]

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.

[MylesBorins]

ping re: backport @danbev

[danbev]

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