deps: fix integration issues with the latest nghttp2

This is a set of src & tests fixes for nghttp2 due to changes in
v1.67.0+ which require a selection of changes to how we handle
low-level protocol errors when using the latest versions of nghttp2,
changing both some src error handling and updating some tests to match.

Signed-off-by: Tim Perry <pimterry@gmail.com>
PR-URL: #62891
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Author: pimterry

src/node_http2.cc

@@ -1270,6 +1270,24 @@ int Http2Session::OnFrameSent(nghttp2_session* handle,
                               void* user_data) {
   Http2Session* session = static_cast<Http2Session*>(user_data);
   session->statistics_.frame_sent += 1;
+
+  // If nghttp2 has internally terminated the session (e.g. due to a protocol
+  // error like oversized frames, padding errors, or HPACK compression
+  // failures), it calls nghttp2_session_terminate_session() directly which
+  // queues a GOAWAY but does not invoke any application-level callback.
+  // Detect that case here: a GOAWAY was sent but we never initiated it
+  // (no Close(), no session.close(), no session.goaway()).
+  //
+  // We set a flag here, and then throw the error at the end of
+  // SendPendingData, to wait until the GOAWAY is written before the session
+  // is torn down.
+  if (frame->hd.type == NGHTTP2_GOAWAY && !session->is_closing() &&
+      !session->is_destroyed() && !session->IsGracefulCloseInitiated() &&
+      !session->goaway_initiated_) {
+    Debug(session, "nghttp2 session terminated internally");
+    session->internal_goaway_sent_ = true;
+  }
+
   return 0;
 }
 
@@ -2004,6 +2022,18 @@ uint8_t Http2Session::SendPendingData() {
 
   MaybeStopReading();
 
+  // If nghttp2 has internally torn down the session (detected in OnFrameSent)
+  // during the nghttp2_session_mem_send loop above, at this point we error:
+  if (internal_goaway_sent_) {
+    internal_goaway_sent_ = false;
+    if (!is_closing() && !is_destroyed()) {
+      Isolate* isolate = env()->isolate();
+      HandleScope scope(isolate);
+      Local<Value> arg = Integer::New(isolate, NGHTTP2_ERR_PROTO);
+      MakeCallback(env()->http2session_on_error_function(), 1, &arg);
+    }
+  }
+
   return 0;
 }
 
@@ -2944,6 +2974,7 @@ void Http2Session::Goaway(uint32_t code,
   if (is_destroyed())
     return;
 
+  goaway_initiated_ = true;
   Http2Scope h2scope(this);
   // the last proc stream id is the most recently created Http2Stream.
   if (lastStreamID <= 0)

src/node_http2.h

@@ -968,6 +968,8 @@ class Http2Session : public AsyncWrap,
 
   // Flag to indicate that JavaScript has initiated a graceful closure
   bool graceful_close_initiated_ = false;
+  bool goaway_initiated_ = false;
+  bool internal_goaway_sent_ = false;
 };
 
 struct Http2SessionPerformanceEntryTraits {

test/parallel/test-http2-misbehaving-flow-control-paused.js

@@ -47,36 +47,45 @@ const data = Buffer.from([
 // Bad client! Bad!
 //
 // Fortunately, nghttp2 handles this situation for us by keeping track
-// of the flow control window and responding with a FLOW_CONTROL_ERROR
-// causing the stream to get shut down...
-//
-// At least, that's what is supposed to happen.
+// of the flow control window and sending GOAWAY to end the session.
 
 let client;
 
 const server = h2.createServer({ settings: { initialWindowSize: 36 } });
-server.on('stream', (stream) => {
+
+server.on('session', common.mustCall((session) => {
+  session.on('error', common.expectsError({
+    code: 'ERR_HTTP2_ERROR',
+    name: 'Error',
+    message: 'Protocol error'
+  }));
+}));
+
+server.on('stream', common.mustCall((stream) => {
   // Set the high water mark to zero, since otherwise we still accept
   // reads from the source stream (if we can consume them).
   stream._readableState.highWaterMark = 0;
   stream.pause();
   stream.on('error', common.expectsError({
-    code: 'ERR_HTTP2_STREAM_ERROR',
+    code: 'ERR_HTTP2_ERROR',
     name: 'Error',
-    message: 'Stream closed with error code NGHTTP2_FLOW_CONTROL_ERROR'
+    message: 'Protocol error'
   }));
   stream.on('close', common.mustCall(() => {
     server.close();
     client.destroy();
   }));
   stream.on('end', common.mustNotCall());
-});
+}));
 
-server.listen(0, () => {
-  client = net.connect(server.address().port, () => {
+server.listen(0, common.mustCall(() => {
+  client = net.connect(server.address().port, common.mustCall(() => {
     client.write(preamble);
     client.write(data);
     client.write(data);
     client.write(data);
-  });
-});
+
+    // TCP connection is closed by the server
+    client.on('close', common.mustCall());
+  }));
+}));

test/parallel/test-http2-misbehaving-flow-control.js

@@ -58,18 +58,24 @@ const data = Buffer.from([
 // Bad client! Bad!
 //
 // Fortunately, nghttp2 handles this situation for us by keeping track
-// of the flow control window and responding with a FLOW_CONTROL_ERROR
-// causing the stream to get shut down...
-//
-// At least, that's what is supposed to happen.
+// of the flow control window and sending GOAWAY to end the session.
 
 let client;
 const server = h2.createServer({ settings: { initialWindowSize: 18 } });
-server.on('stream', (stream) => {
+
+server.on('session', common.mustCall((session) => {
+  session.on('error', common.expectsError({
+    code: 'ERR_HTTP2_ERROR',
+    name: 'Error',
+    message: 'Protocol error'
+  }));
+}));
+
+server.on('stream', common.mustCall((stream) => {
   stream.on('error', common.expectsError({
-    code: 'ERR_HTTP2_STREAM_ERROR',
+    code: 'ERR_HTTP2_ERROR',
     name: 'Error',
-    message: 'Stream closed with error code NGHTTP2_FLOW_CONTROL_ERROR'
+    message: 'Protocol error'
   }));
   stream.on('close', common.mustCall(() => {
     server.close(common.mustCall());
@@ -78,14 +84,17 @@ server.on('stream', (stream) => {
   stream.resume();
   stream.respond();
   stream.end('ok');
-});
+}));
 
 server.on('close', common.mustCall());
 
-server.listen(0, () => {
-  client = net.connect(server.address().port, () => {
+server.listen(0, common.mustCall(() => {
+  client = net.connect(server.address().port, common.mustCall(() => {
     client.write(preamble);
     client.write(data);
     client.write(data);
-  });
-});
+
+    // TCP connection is closed by the server
+    client.on('close', common.mustCall());
+  }));
+}));

test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js

@@ -90,11 +90,19 @@ server.listen(0, common.mustCall(() => {
     0,
     common.mustCall(() => {
       const client = h2.connect(`http://localhost:${server.address().port}`);
-      client.on('error', common.mustNotCall());
+      // The server sends oversized headers that cause a compression error on
+      // the client side, so nghttp2 internally terminates the client session.
+      client.on('error', common.expectsError({
+        code: 'ERR_HTTP2_ERROR',
+        name: 'Error',
+      }));
 
       const req = client.request();
       req.on('response', common.mustNotCall());
-      req.on('error', common.mustNotCall());
+      req.on('error', common.expectsError({
+        code: 'ERR_HTTP2_ERROR',
+        name: 'Error',
+      }));
       req.end();
     }),
   );

test/parallel/test-http2-session-cleanup-on-nghttp2-goaway.js

@@ -0,0 +1,91 @@
+'use strict';
+
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const http2 = require('http2');
+const net = require('net');
+
+// When nghttp2 internally sends a GOAWAY frame due to a protocol error, it
+// may call nghttp2_session_terminate_session() directly, bypassing the
+// on_invalid_frame_recv_callback entirely. This test ensures that even
+// in that scenario, we still correctly clean up the session & connection.
+//
+// This test reproduces this with a client who sends a frame header with
+// a length exceeding the default max_frame_size (16384). nghttp2 responds
+// with GOAWAY(FRAME_SIZE_ERROR) without notifying Node through any callback.
+
+const server = http2.createServer();
+
+server.on('session', common.mustCall((session) => {
+  session.on('error', common.expectsError({
+    code: 'ERR_HTTP2_ERROR',
+    name: 'Error',
+    message: 'Protocol error'
+  }));
+
+  session.on('close', common.mustCall(() => server.close()));
+}));
+
+server.listen(0, common.mustCall(() => {
+  const conn = net.connect({
+    port: server.address().port,
+    allowHalfOpen: true,
+  });
+
+  // HTTP/2 client connection preface.
+  conn.write('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n');
+
+  // Empty SETTINGS frame.
+  const settingsFrame = Buffer.alloc(9);
+  settingsFrame[3] = 0x04; // type: SETTINGS
+  conn.write(settingsFrame);
+
+  let inbuf = Buffer.alloc(0);
+  let state = 'settingsHeader';
+  let settingsFrameLength;
+
+  conn.on('data', (chunk) => {
+    inbuf = Buffer.concat([inbuf, chunk]);
+
+    switch (state) {
+      case 'settingsHeader':
+        if (inbuf.length < 9) return;
+        settingsFrameLength = inbuf.readUIntBE(0, 3);
+        inbuf = inbuf.slice(9);
+        state = 'readingSettings';
+        // Fallthrough
+      case 'readingSettings': {
+        if (inbuf.length < settingsFrameLength) return;
+        inbuf = inbuf.slice(settingsFrameLength);
+        state = 'done';
+
+        // ACK the server SETTINGS.
+        const ack = Buffer.alloc(9);
+        ack[3] = 0x04; // type: SETTINGS
+        ack[4] = 0x01; // flag: ACK
+        conn.write(ack);
+
+        // Send a HEADERS frame header claiming length 16385, which exceeds
+        // the default max_frame_size of 16384. nghttp2 checks the length
+        // before reading any payload, so no body is needed. This triggers
+        // nghttp2_session_terminate_session(FRAME_SIZE_ERROR) directly in
+        // nghttp2_session_mem_recv2 — bypassing on_invalid_frame_recv_callback.
+        const oversized = Buffer.alloc(9);
+        oversized.writeUIntBE(16385, 0, 3); // length: 16385 (one over max)
+        oversized[3] = 0x01;                // type: HEADERS
+        oversized[4] = 0x04;                // flags: END_HEADERS
+        oversized.writeUInt32BE(1, 5);       // stream id: 1
+        conn.write(oversized);
+
+        // No need to write the data - the header alone triggers the check.
+      }
+    }
+  });
+
+  // The server must close the connection after sending GOAWAY:
+  conn.on('end', common.mustCall(() => conn.end()));
+  conn.on('close', common.mustCall());
+}));