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 <anna@addaleax.net>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

3 files changed, 100 insertions, 35 deletions

diff --git a/src/quic/node_quic_crypto.cc b/src/quic/node_quic_crypto.cc
index ac5cae6e548..8f1f433525e 100644
--- a/src/quic/node_quic_crypto.cc
+++ b/src/quic/node_quic_crypto.cc
@@ -351,8 +351,14 @@ Local<Value> GetALPNProtocol(const QuicSession& session) {
 namespace {
 int CertCB(SSL* ssl, void* arg) {
   QuicSession* session = static_cast<QuicSession*>(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<QuicSession*>(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 c2c4172393c..c306226f0c8 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<Array> ciphers;
   Local<Value> alpn_string = Undefined(env->isolate());
   Local<Value> 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<Value> 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<Value> argv[] = {
+      alpn_string,
+      servername,
+      ciphers
+    };
 
-  // Grab a shared pointer to this to prevent the QuicSession
-  // from being freed while the MakeCallback is running.
-  BaseObjectPtr<QuicSession> 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<QuicSession> 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<Value> 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<QuicSession> 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 165a6187205..c41d58125b9 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<QuicSession> session_;
+  std::unique_ptr<InternalCallbackScope> private_;
+  v8::TryCatch try_catch_;
+};
+
 }  // namespace quic
 }  // namespace node