http2: fix write deadlock exposed by larger window sizes

This removes a guard (no reads while write pending) that creates this deadlock, which was added as a security mechanism. This guard is redundant given then other existing mechanisms, and a test is added to demonstrate that. Signed-off-by: Tim Perry <pimterry@gmail.com>
nodejs-github-bot · Aug 26, 2026 · Aug 20, 2026 · Aug 20, 2026 · Aug 21, 2026 · Aug 24, 2026
commit 46498569d2606076c3c4b66b9d4dc0ee84b35300
diff --git a/benchmark/http2/full-duplex.js b/benchmark/http2/full-duplex.js
@@ -0,0 +1,71 @@
+'use strict';
+
+const common = require('../common.js');
+const fixtures = require('../../test/common/fixtures');
+
+const bench = common.createBenchmark(main, {
+  n: [100],
+  streams: [2],
+  size: [4 * 1024 * 1024],
+  // Use the HTTP/2 protocol default.
+  window: [65535],
+}, {
+  test: { size: 128 * 1024, window: 65535 },
+});
+
+function main({ n, streams, size, window }) {
+  const http2 = require('http2');
+  const payload = Buffer.alloc(size);
+  const server = http2.createSecureServer({
+    key: fixtures.readKey('agent1-key.pem'),
+    cert: fixtures.readKey('agent1-cert.pem'),
+    settings: { initialWindowSize: window },
+  });
+
+  let completed = 0;
+  let batches = 0;
+
+  function onTransferComplete() {
+    if (++completed !== streams * 2)
+      return;
+
+    if (++batches === n) {
+      // Report combined upload and download throughput in MiB/s.
+      bench.end(n * streams * size * 2 / (1024 * 1024));
+      client.close();
+      server.close();
+      return;
+    }
+
+    startBatch();
+  }
+
+  server.on('stream', (stream) => {
+    stream.resume();
+    stream.on('end', onTransferComplete);
+    stream.respond();
+    stream.end(payload);
+  });
+
+  let client;
+  function startBatch() {
+    completed = 0;
+    for (let i = 0; i < streams; i++) {
+      const request = client.request({ ':method': 'POST' });
+      request.resume();
+      request.on('end', onTransferComplete);
+      request.end(payload);
+    }
+  }
+
+  server.listen(0, () => {
+    client = http2.connect(`https://localhost:${server.address().port}`, {
+      rejectUnauthorized: false,
+      settings: { initialWindowSize: window },
+    });
+    client.on('connect', () => {
+      bench.start();
+      startBatch();
+    });
+  });
+}
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -1485,15 +1485,6 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle,
     }
   } while (len != 0);
 
-  // If we are currently waiting for a write operation to finish, we should
-  // tell nghttp2 that we want to wait before we process more input data.
-  if (session->is_write_in_progress()) {
-    CHECK(session->is_reading_stopped());
-    session->set_receive_paused();
-    Debug(session, "receive paused");
-    return NGHTTP2_ERR_PAUSE;
-  }
-
   return 0;
 }
 
@@ -1942,7 +1933,7 @@ void Http2Session::MaybeStopReading() {
   if (is_reading_stopped() || is_closing()) return;
   int want_read = nghttp2_session_want_read(session_.get());
   Debug(this, "wants read? %d", want_read);
-  if (want_read == 0 || is_write_in_progress()) {
+  if (want_read == 0) {
     set_reading_stopped();
     stream_->ReadStop();
   }

diff --git a/test/parallel/test-http2-bidirectional-write-deadlock.js b/test/parallel/test-http2-bidirectional-write-deadlock.js
@@ -0,0 +1,107 @@
+'use strict';
+
+// Regression test against deadlocks between two HTTP/2 peers that are both
+// writing at the same time.
+//
+// To bound how much it buffered while output was backed up, an Http2Session
+// used to stop reading from its socket whenever a write was in flight, and
+// resume only once that write completed. When the peer was itself blocked
+// writing, that write never completed, so the session never read again and
+// the connection hung forever with no error and no timeout.
+//
+// Rather than relying on kernel socket buffers filling up - which depends on
+// the platform and configured window sizes - this models one half of that
+// cycle directly. The client's socket forwards a write but does not report it
+// as complete, substituting for a write blocked because the peer is not
+// reading. Only after that write is stalled does the server send its response
+// body. A session that stops reading while writing never sees it.
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const http2 = require('http2');
+const net = require('net');
+const { Duplex } = require('stream');
+
+const BODY = 'the response body';
+
+const heldCallbacks = [];
+let serverStream;
+
+let stallWrites = false;
+
+// Client-side socket that forwards writes to a real connection, but can leave
+// their completion callbacks pending to model a transport-blocked write.
+class StalledClientSocket extends Duplex {
+  constructor(port) {
+    super();
+    this.inner = net.connect(port, common.localhostIPv4);
+    this.inner.on('data', (chunk) => this.push(chunk));
+  }
+  _read() {
+    // Incoming data is pushed as it arrives.
+  }
+  _write(chunk, encoding, callback) {
+    this.inner.write(chunk, encoding);
+    if (stallWrites) {
+      heldCallbacks.push(callback);
+      // Avoid writing from the server re-entrantly inside _write(). The
+      // ordering is still explicit: this callback is already held.
+      setImmediate(() => serverStream.end(BODY));
+      return;
+    }
+    callback();
+  }
+  _final(callback) {
+    callback();
+  }
+  _destroy(err, callback) {
+    this.inner.destroy();
+    callback(err);
+  }
+}
+
+const server = http2.createServer();
+
+server.on('stream', common.mustCall((stream) => {
+  // Send headers first. Their response event starts the stalled client write.
+  stream.respond();
+  serverStream = stream;
+}));
+
+server.listen(0, common.mustCall(() => {
+  const port = server.address().port;
+
+  const client = http2.connect(`http://${common.localhostIPv4}:${port}`, {
+    createConnection: () => new StalledClientSocket(port),
+  });
+
+  const req = client.request({ ':method': 'POST' });
+
+  let received = '';
+
+  req.on('response', common.mustCall(() => {
+    // _write() will schedule the response body only after it has retained the
+    // callback, guaranteeing that the native write is still in progress.
+    stallWrites = true;
+    req.write(Buffer.alloc(256));
+  }));
+
+  req.on('data', (chunk) => {
+    received += chunk;
+  });
+
+  req.on('end', common.mustCall(() => {
+    assert.strictEqual(received, BODY);
+    assert.ok(heldCallbacks.length > 0,
+              'test did not actually stall a socket write');
+
+    // Let the stalled writes complete so that everything can shut down.
+    stallWrites = false;
+    for (const callback of heldCallbacks) callback();
+
+    client.destroy();
+    server.close();
+  }));
+}));
diff --git a/test/parallel/test-http2-session-memory-unread-peer.js b/test/parallel/test-http2-session-memory-unread-peer.js
@@ -0,0 +1,100 @@
+'use strict';
+
+// CVE-2019-9517 describes a peer that advertises HTTP/2 flow-control credit
+// but does not drain its TCP socket, leaving the server holding responses it
+// cannot write out.
+//
+// Node bounds this with maxSessionMemory. Once the budget is spent, further
+// streams are refused rather than queued, so a client cannot make the server
+// buffer a response for every request it opens.
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const http2 = require('http2');
+const net = require('net');
+const { Duplex } = require('stream');
+
+const TOTAL_REQUESTS = 200;
+// Accepted count depends on kernel buffer sizes but this seems to
+// cover most likely scenarios:
+const MAX_ACCEPTED = 20;
+const body = Buffer.alloc(1_000_000);
+
+let accepted = 0;
+let client;
+
+// Client-side socket that forwards writes but never consumes incoming data.
+class UnreadClientSocket extends Duplex {
+  constructor(port) {
+    super();
+    this.inner = net.connect(port, common.localhostIPv4);
+    this.inner.pause();
+  }
+  _read() {
+    // Deliberately never pull from the underlying socket.
+  }
+  _write(chunk, encoding, callback) {
+    this.inner.write(chunk, encoding, callback);
+  }
+  _final(callback) {
+    callback();
+  }
+  _destroy(err, callback) {
+    this.inner.destroy();
+    callback(err);
+  }
+}
+
+const onStreamsRefused = common.mustCall(() => {
+  assert.ok(accepted >= 1, 'client never reached the server');
+  client.destroy();
+  server.close();
+});
+
+const server = http2.createServer({
+  maxSessionMemory: 1,
+  // Turn the rejected streams into an observable server-session failure.
+  maxSessionRejectedStreams: 0,
+});
+
+server.on('session', common.mustCall((session) => {
+  session.on('error', common.mustCall((err) => {
+    assert.strictEqual(err.code, 'ERR_HTTP2_ERROR');
+    onStreamsRefused();
+  }));
+}));
+
+// At least one stream must be accepted, otherwise the assertion above could
+// pass without the client ever having reached the server.
+server.on('stream', common.mustCallAtLeast((stream) => {
+  accepted++;
+  // Assert here rather than once the session fails: if the bound does not
+  // hold there may be no session failure at all.
+  assert.ok(accepted <= MAX_ACCEPTED,
+            `server accepted ${accepted} of ${TOTAL_REQUESTS} streams ` +
+            'from a peer that never reads; maxSessionMemory did not ' +
+            'bound its outbound buffering');
+  // The fatal session error also destroys every accepted stream.
+  stream.on('error', common.mustCall((err) => {
+    assert.strictEqual(err.code, 'ERR_HTTP2_ERROR');
+  }));
+  stream.respond();
+  stream.end(body);
+}));
+
+server.listen(0, common.mustCall(() => {
+  const port = server.address().port;
+
+  client = http2.connect(`http://${common.localhostIPv4}:${port}`, {
+    createConnection: () => new UnreadClientSocket(port),
+  });
+
+  client.on('connect', common.mustCall(() => {
+    for (let i = 0; i < TOTAL_REQUESTS; i++) {
+      const req = client.request();
+      req.end();
+    }
+  }));
+}));
diff --git a/test/parallel/test-stream-pipeline-http2.js b/test/parallel/test-stream-pipeline-http2.js
@@ -30,8 +30,7 @@ const http2 = require('http2');
     let received = 0;
     req.on('data', (data) => {
       received += data.length;
-      // Bound the data that flows before teardown - bytes per data event vary
-      // by platform, and letting this run longer hangs on macOS.
+      // Use a byte threshold because data event chunking varies by platform.
       if (received >= 32 * 1024) rs.destroy();
     });
   }));