From d96083bad56e97ddef525c2c0db6963d70fd6f6c Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 29 Jul 2020 11:27:14 -0700 Subject: [PATCH] quic: introduce QuicCallbackScope Alternative to `CallbackScope` that handles destroying the `QuicSession` in the try_catch cleanup. PR-URL: https://github.com/nodejs/node/pull/34541 Reviewed-By: Anna Henningsen Reviewed-By: Jiawen Geng --- src/quic/node_quic_crypto.cc | 14 ++++- src/quic/node_quic_session.cc | 105 +++++++++++++++++++++++----------- src/quic/node_quic_session.h | 16 ++++++ 3 files changed, 100 insertions(+), 35 deletions(-) diff --git a/src/quic/node_quic_crypto.cc b/src/quic/node_quic_crypto.cc index ac5cae6e548ed4..8f1f433525ecc3 100644 --- a/src/quic/node_quic_crypto.cc +++ b/src/quic/node_quic_crypto.cc @@ -351,8 +351,14 @@ Local GetALPNProtocol(const QuicSession& session) { namespace { int CertCB(SSL* ssl, void* arg) { QuicSession* session = static_cast(arg); - return SSL_get_tlsext_status_type(ssl) == TLSEXT_STATUSTYPE_ocsp ? - session->crypto_context()->OnOCSP() : 1; + int ret; + switch (SSL_get_tlsext_status_type(ssl)) { + case TLSEXT_STATUSTYPE_ocsp: + ret = session->crypto_context()->OnOCSP(); + return UNLIKELY(session->is_destroyed()) ? 0 : ret; + default: + return 1; + } } void Keylog_CB(const SSL* ssl, const char* line) { @@ -366,6 +372,10 @@ int Client_Hello_CB( void* arg) { QuicSession* session = static_cast(SSL_get_app_data(ssl)); int ret = session->crypto_context()->OnClientHello(); + if (UNLIKELY(session->is_destroyed())) { + *tls_alert = SSL_R_SSL_HANDSHAKE_FAILURE; + return 0; + } switch (ret) { case 0: return 1; diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index c2c4172393cefa..c306226f0c8913 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -57,8 +57,40 @@ using v8::String; using v8::Undefined; using v8::Value; +using TryCatchScope = node::errors::TryCatchScope; + namespace quic { +QuicCallbackScope::QuicCallbackScope(QuicSession* session) + : session_(session), + private_(new InternalCallbackScope( + session->env(), + session->object(), + { + session->get_async_id(), + session->get_trigger_async_id() + })), + try_catch_(session->env()->isolate()) { + try_catch_.SetVerbose(true); +} + +QuicCallbackScope::~QuicCallbackScope() { + Environment* env = session_->env(); + if (UNLIKELY(try_catch_.HasCaught())) { + session_->crypto_context()->set_in_client_hello(false); + session_->crypto_context()->set_in_ocsp_request(false); + if (!try_catch_.HasTerminated() && env->can_call_into_js()) { + session_->set_last_error({ + QUIC_ERROR_SESSION, + uint64_t{NGTCP2_INTERNAL_ERROR} + }); + session_->Close(); + CHECK(session_->is_destroyed()); + } + private_->MarkAsFailed(); + } +} + typedef ssize_t(*ngtcp2_close_fn)( ngtcp2_conn* conn, ngtcp2_path* path, @@ -393,34 +425,36 @@ void JSQuicSessionListener::OnClientHello( HandleScope scope(env->isolate()); Context::Scope context_scope(env->context()); + // Why this instead of using MakeCallback? We need to catch any + // errors that happen both when preparing the arguments and + // invoking the callback so that we can properly signal a failure + // to the peer. + QuicCallbackScope cb_scope(session()); + Local ciphers; Local alpn_string = Undefined(env->isolate()); Local servername = Undefined(env->isolate()); - // TODO(@jasnell): Need to decide how to handle the possible - // ToLocal failures more gracefully than crashing. - - CHECK(session()->crypto_context()->hello_ciphers().ToLocal(&ciphers)); - - if (alpn != nullptr) - CHECK(String::NewFromUtf8(env->isolate(), alpn).ToLocal(&alpn_string)); - - if (server_name != nullptr) - CHECK(String::NewFromUtf8(env->isolate(), server_name) - .ToLocal(&servername)); - - Local argv[] = { - alpn_string, - servername, - ciphers - }; + if (session()->crypto_context()->hello_ciphers().ToLocal(&ciphers) && + (alpn == nullptr || + String::NewFromUtf8(env->isolate(), alpn).ToLocal(&alpn_string)) && + (server_name == nullptr || + String::NewFromUtf8(env->isolate(), server_name).ToLocal(&servername))) { + Local argv[] = { + alpn_string, + servername, + ciphers + }; - // Grab a shared pointer to this to prevent the QuicSession - // from being freed while the MakeCallback is running. - BaseObjectPtr ptr(session()); - session()->MakeCallback( - env->quic_on_session_client_hello_function(), - arraysize(argv), argv); + // Grab a shared pointer to this to prevent the QuicSession + // from being freed while the MakeCallback is running. + BaseObjectPtr ptr(session()); + env->quic_on_session_client_hello_function()->Call( + env->context(), + session()->object(), + arraysize(argv), + argv); + } } void JSQuicSessionListener::OnCert(const char* server_name) { @@ -428,18 +462,22 @@ void JSQuicSessionListener::OnCert(const char* server_name) { HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); + QuicCallbackScope cb_scope(session()); + Local servername = Undefined(env->isolate()); - if (server_name != nullptr) { - servername = OneByteString( - env->isolate(), - server_name, - strlen(server_name)); - } - // Grab a shared pointer to this to prevent the QuicSession - // from being freed while the MakeCallback is running. BaseObjectPtr ptr(session()); - session()->MakeCallback(env->quic_on_session_cert_function(), 1, &servername); + if (server_name == nullptr || + String::NewFromUtf8( + env->isolate(), + server_name, + v8::NewStringType::kNormal, + strlen(server_name)).ToLocal(&servername)) { + env->quic_on_session_cert_function()->Call( + env->context(), + session()->object(), + 1, &servername); + } } void JSQuicSessionListener::OnStreamHeaders( @@ -2913,11 +2951,12 @@ int QuicSession::OnReceiveCryptoData( return NGTCP2_ERR_CALLBACK_FAILURE; QuicSession::NgCallbackScope callback_scope(session); - return session->crypto_context()->Receive( + int ret = session->crypto_context()->Receive( crypto_level, offset, data, datalen); + return ret == 0 ? 0 : NGTCP2_ERR_CALLBACK_FAILURE; } // Called by ngtcp2 for both client and server connections diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index 165a61872052f5..c41d58125b9f70 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -1507,6 +1507,22 @@ class QuicSession final : public AsyncWrap, friend class JSQuicSessionListener; }; +class QuicCallbackScope { + public: + explicit QuicCallbackScope(QuicSession* session); + ~QuicCallbackScope(); + + void operator=(const QuicCallbackScope&) = delete; + void operator=(QuicCallbackScope&&) = delete; + QuicCallbackScope(const QuicCallbackScope&) = delete; + QuicCallbackScope(QuicCallbackScope&&) = delete; + + private: + BaseObjectPtr session_; + std::unique_ptr private_; + v8::TryCatch try_catch_; +}; + } // namespace quic } // namespace node