quic: fix subtle bugs in resume & failed H3 start teardown

And a little related cleanup en route

Signed-off-by: Tim Perry <pimterry@gmail.com>

Author: pimterry

src/quic/endpoint.cc

@@ -1997,7 +1997,12 @@ void Endpoint::SocketAddressInfoTraits::Touch(const SocketAddress& address,
 // JavaScript call outs
 
 void Endpoint::EmitNewSession(const BaseObjectPtr<Session>& session) {
-  if (!env()->can_call_into_js()) return;
+  if (!env()->can_call_into_js()) {
+    // Even if we can't call into JS, we need to attach the app to handle
+    // other callbacks before we do proper teardown:
+    session->EnsureApplication();
+    return;
+  }
   CallbackScope<Endpoint> scope(this);
   session->set_wrapped();
   Local<Value> arg = session->object();

src/quic/session.cc

@@ -2629,7 +2629,7 @@ bool Session::EnsureApplication() {
   if (is_destroyed()) [[unlikely]]
     return false;
   if (impl_->application_) [[likely]]
-    return true;
+    return !flags_.application_start_failed;
 
   if (application_type() == Application::Type::HTTP3) {
     SetApplication(CreateHttp3Application(this));
@@ -2639,9 +2639,16 @@ bool Session::EnsureApplication() {
   }
 
   // If the keys are already ready, that means we should start immediately.
-  // If application start fails then we can't continue.
+  // If application start fails then we can't continue. Inside an ngtcp2
+  // callback the session can't be closed directly, but the failure sticks,
+  // and HandshakeCompleted() then fails the callback, which closes it.
   if (keys_ready_ && !application().Start()) {
     Debug(this, "Application start failed");
+    flags_.application_start_failed = 1;
+    if (!flags_.in_ngtcp2_callback_scope) {
+      SetLastError(QuicError::ForNgtcp2Error(NGTCP2_ERR_INTERNAL));
+      Close();
+    }
     return false;
   }
   return true;
@@ -2865,7 +2872,7 @@ bool Session::AfterNgtcp2Read(int err) {
         if (is_server() && tls_session().early_selection() ==
                                TLSSession::EarlySelection::kSelected) {
           endpoint().EmitNewSession(BaseObjectPtr<Session>(this));
-          if (!is_destroyed()) ResumeHandshake();
+          if (has_application()) ResumeHandshake();
         }
       }
       return true;
@@ -3408,15 +3415,16 @@ void Session::StreamDataBlocked(stream_id id) {
 
 void Session::CollectSessionTicketAppData(
     SessionTicket::AppData* app_data) const {
-  DCHECK(!is_destroyed());
-  CHECK(has_application());
+  if (!has_application()) [[unlikely]]
+    return;
   application().CollectSessionTicketAppData(app_data);
 }
 
 SessionTicket::AppData::Status Session::ExtractSessionTicketAppData(
     const SessionTicket::AppData& app_data, Flag flag) {
-  DCHECK(!is_destroyed());
-  CHECK(has_application());
+  if (!has_application()) [[unlikely]] {
+    return SessionTicket::AppData::Status::TICKET_IGNORE_RENEW;
+  }
   return application().ExtractSessionTicketAppData(app_data, flag);
 }
 

src/quic/session.h

@@ -11,7 +11,6 @@
 #include <node_sockaddr.h>
 #include <timer_wrap.h>
 #include <util.h>
-#include <memory>
 #include <optional>
 #include <span>
 #include "bindingdata.h"
@@ -741,6 +740,8 @@ class Session final : public AsyncWrap, private SessionTicket::AppData::Source {
     // Set during FlushPendingData to avoid the one-tick latency of
     // async-only sends from the uv_check callback.
     uint8_t prefer_try_send : 1 = 0;
+    // Set if the application couldn't be started, which is fatal to it.
+    uint8_t application_start_failed : 1 = 0;
   };
   Flags flags_;
 

test/parallel/test-quic-alpn-h3.mjs

@@ -49,5 +49,6 @@ assert.throws(() => stream.sendHeaders({ ':status': '200' }), {
   message: /does not support headers/,
 });
 
-clientSession.destroy();
+stream.destroy();
+await clientSession.close();
 await serverEndpoint.close();

test/parallel/test-quic-h3-attach.mjs

@@ -61,10 +61,6 @@ assert.throws(() => new Http3Session(), { code: 'ERR_ILLEGAL_CONSTRUCTOR' });
       code: 'ERR_INVALID_STATE',
       message: /cannot be set on a session/,
     });
-    // And the HTTP/3-only callbacks exist only there:
-    for (const name of ['ongoaway', 'onorigin', 'onapplication']) {
-      assert.strictEqual(name in quicSession, false);
-    }
     // The onerror callback stays transport-level, so both sides keep their own:
     quicSession.onerror = () => {};
     session.onerror = () => {};
@@ -96,20 +92,6 @@ const tooLate = {
   message: /already has an application/,
 };
 
-// HTTP/3-only callbacks can't be passed as QuicSession options, so they
-// can't be registered before the application exists:
-for (const name of ['ongoaway', 'onorigin', 'onapplication']) {
-  const expected = {
-    code: 'ERR_INVALID_ARG_VALUE',
-    message: new RegExp(`options\\.${name}.*Http3Session`),
-  };
-  const callback = { [name]: () => {} };
-  await assert.rejects(listen(() => {}, { ...serverOpts, ...callback }),
-                       expected);
-  await assert.rejects(connect('127.0.0.1:1', { ...clientOpts, ...callback }),
-                       expected);
-}
-
 // Setting onstream claims the session for raw QUIC, so HTTP/3 can't be
 // attached afterwards, whether it is set directly or passed as an option.
 {

test/parallel/test-quic-h3-start-failure.mjs

@@ -0,0 +1,68 @@
+// Flags: --experimental-quic --no-warnings
+
+// Test: HTTP/3 can't start when the peer allows fewer than the three
+// unidirectional streams it needs for its control and QPACK streams. That
+// must close the session, rather than leave it running without HTTP/3.
+
+import { hasQuic, skip, mustCall } from '../common/index.mjs';
+import assert from 'node:assert';
+import * as fixtures from '../common/fixtures.mjs';
+
+if (!hasQuic) {
+  skip('QUIC is not enabled');
+}
+
+const { listen, connect } = await import('node:quic');
+const { createPrivateKey } = await import('node:crypto');
+
+const key = createPrivateKey(fixtures.readKey('agent1-key.pem'));
+const cert = fixtures.readKey('agent1-cert.pem');
+const headers = {
+  ':method': 'GET',
+  ':path': '/',
+  ':scheme': 'https',
+  ':authority': 'localhost',
+};
+const internalError = {
+  code: 'ERR_QUIC_TRANSPORT_ERROR',
+  message: /INTERNAL_ERROR/,
+};
+
+async function attachToLowUniServer() {
+  const endpoint = await listen(mustCall((quicSession) => {
+    quicSession.onerror = () => {};
+  }), {
+    alpn: ['h3'],
+    sni: { '*': { keys: [key], certs: [cert] } },
+    transportParams: { initialMaxStreamsUni: 2 },
+  });
+  const client = await connect(endpoint.address, {
+    alpn: 'h3',
+    servername: 'localhost',
+    verifyPeer: 'manual',
+  });
+  return { endpoint, client };
+}
+
+// Nothing opened: the session closes when the handshake completes.
+{
+  const { endpoint, client } = await attachToLowUniServer();
+  await assert.rejects(client.closed, internalError);
+  await endpoint.close();
+}
+
+// A request opened as soon as the session opens fails, and the session
+// closes rather than accepting further requests.
+{
+  const { endpoint, client } = await attachToLowUniServer();
+  client.onerror = mustCall((err) => {
+    assert.strictEqual(err.code, internalError.code);
+  });
+  await client.opened;
+  await assert.rejects(client.createBidirectionalStream({ headers }),
+                       { code: 'ERR_QUIC_OPEN_STREAM_FAILED' });
+  await assert.rejects(client.closed, internalError);
+  await assert.rejects(client.createBidirectionalStream({ headers }),
+                       { code: 'ERR_INVALID_STATE' });
+  await endpoint.close();
+}