crypto: check SecureContext existence

[sam-github]

SecureContext should always be present, CHECK() it, on theory an explicit crash is better than one due to null pointer deref (if that is even reachable).

@cjihrig @bnoordhuis ?

Fix:

*** CID 179169: Null pointer dereferences (NULL_RETURNS)
/src/node_crypto.cc: 1353 in node::crypto::SecureContext::SetClientCertEngine(const v8::FunctionCallbackInfov8::Value &)()
1347
1348 // SSL_CTX_set_client_cert_engine does not itself support multiple
1349 // calls by cleaning up before overwriting the client_cert_engine
1350 // internal context variable.
1351 // Instead of trying to fix up this problem we in turn also do not
1352 // support multiple calls to SetClientCertEngine.

CID 179169:  Null pointer dereferences  (NULL_RETURNS)
Dereferencing a null pointer "sc".

1353 if (sc->client_cert_engine_provided_) {
1354 return env->ThrowError(
1355 "Multiple calls to SetClientCertEngine are not allowed");
1356 }
1357
1358 const node::Utf8Value engine_id(env->isolate(), args[0]);

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

[addaleax]

By the way, we usually prefer explicit nullptr checks, i.e. CHECK_NE(sc, nullptr);

[sam-github]

Hah, can you tell we're in the same timezone still, Anna? :-)

[sam-github]

Rewrote as CHECK_NE(), will redo PR test later, I'm about to fly home.

[addaleax]

@sam-github have a safe trip home!

[bnoordhuis]

Oh, hah... #16965. Guess I should have checked my github notifications first.

ASSIGN_OR_RETURN_UNWRAP() is the right thing to use here.

[bnoordhuis]

I went ahead and landed #16965 (commit 6c76140.) Thanks anyway, Sam.

[sam-github]

No problem, thanks Ben.