Skip to content

Commit 3cb04f3

Browse files
gabedemesaclaude
andcommitted
http2: enforce maxSessionMemory for client requests
The documentation for `maxSessionMemory` says new streams are rejected while the limit is exceeded. That was only enforced for streams opened by the peer (`Http2Session::CanAddStream()`); `session.request()` went straight to `nghttp2_submit_request()`. A client whose socket stopped draining could therefore keep submitting requests and queue frames in `outgoing_buffers_` without bound. Check the session memory before submitting a request and fail only that request with `ERR_HTTP2_STREAM_ERROR` (`NGHTTP2_ENHANCE_YOUR_CALM`), leaving the session usable. As with response headers, request bodies queued for flow control are not counted, so large uploads do not block unrelated requests on the same session. Also count the `outgoing_buffers_` descriptors towards the session memory; each queued write costs 32 bytes regardless of its length. Refs: nodejs/undici#6181 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
1 parent 75d55af commit 3cb04f3

4 files changed

Lines changed: 117 additions & 0 deletions

File tree

‎lib/internal/http2/core.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,7 @@ const {
295295

296296
const {
297297
NGHTTP2_CANCEL,
298+
NGHTTP2_ENHANCE_YOUR_CALM,
298299
NGHTTP2_REFUSED_STREAM,
299300
NGHTTP2_DEFAULT_WEIGHT,
300301
NGHTTP2_FLAG_END_STREAM,
@@ -886,6 +887,13 @@ function requestOnConnect(headersList, options) {
886887
// For the first two, emit the error on the session,
887888
// For the third, emit the error on the stream, it will bubble up to the
888889
// session if not handled.
890+
if (ret === false) {
891+
// The session is over its `maxSessionMemory` budget. Only this request is
892+
// rejected; the session itself stays usable.
893+
this.destroy(new ERR_HTTP2_STREAM_ERROR(
894+
nameForErrorCode[NGHTTP2_ENHANCE_YOUR_CALM]));
895+
return;
896+
}
889897
if (typeof ret === 'number') {
890898
let err;
891899
switch (ret) {

‎src/node_http2.cc‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3073,6 +3073,19 @@ void Http2Session::Request(const FunctionCallbackInfo<Value>& args) {
30733073
return;
30743074
}
30753075

3076+
// maxSessionMemory is documented to reject new streams while the limit is
3077+
// exceeded. CanAddStream() enforces that for peer-initiated streams; apply
3078+
// the same memory check to locally initiated requests so that a session
3079+
// whose socket is not draining cannot queue frames without bound. Like the
3080+
// response header check, queued request bodies are not counted, so large
3081+
// uploads do not prevent unrelated requests on the same session.
3082+
if (!session->has_available_session_memory(sizeof(Http2Stream),
3083+
/* exclude_outbound */ true))
3084+
[[unlikely]] {
3085+
Debug(session, "not submitting request: session memory limit exceeded");
3086+
return args.GetReturnValue().Set(false);
3087+
}
3088+
30763089
Debug(session, "request submitted");
30773090

30783091
int32_t ret = 0;

‎src/node_http2.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -832,6 +832,7 @@ class Http2Session : public AsyncWrap,
832832
uint64_t total = current_session_memory_ + sizeof(Http2Session);
833833
total += current_nghttp2_memory_;
834834
total += outgoing_storage_.size();
835+
total += outgoing_buffers_.size() * sizeof(NgHttp2StreamWrite);
835836
return total;
836837
}
837838

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const assert = require('assert');
8+
const http2 = require('http2');
9+
const { once } = require('events');
10+
11+
const { NGHTTP2_ENHANCE_YOUR_CALM } = http2.constants;
12+
13+
// maxSessionMemory is documented to reject new streams while the limit is
14+
// exceeded. Make sure that also holds for requests initiated by a client
15+
// session, so that a peer that does not read cannot make the client queue
16+
// frames without bound. As for response headers, request bodies that are
17+
// queued waiting for flow control are not counted, so a large upload does not
18+
// prevent unrelated requests on the same session.
19+
20+
async function queuedUploadDoesNotBlockRequests() {
21+
// The server never opens its flow-control window, so the upload stays queued
22+
// in the client session.
23+
const server = http2.createServer({ settings: { initialWindowSize: 0 } });
24+
server.on('stream', common.mustCall((stream) => {
25+
stream.on('error', () => {});
26+
}, 2));
27+
server.listen(0);
28+
await once(server, 'listening');
29+
30+
const client = http2.connect(`http://localhost:${server.address().port}`, {
31+
maxSessionMemory: 1,
32+
});
33+
client.on('error', common.mustNotCall());
34+
await once(client, 'remoteSettings');
35+
36+
const uploadArrived = once(server, 'stream');
37+
const upload = client.request({ ':method': 'POST', ':path': '/upload' });
38+
upload.on('error', () => {});
39+
upload.end(Buffer.alloc(4 * 1024 * 1024));
40+
await uploadArrived;
41+
42+
const other = client.request({ ':path': '/other' });
43+
other.on('error', () => {});
44+
await once(server, 'stream');
45+
46+
other.destroy();
47+
upload.destroy();
48+
client.destroy();
49+
server.close();
50+
}
51+
52+
async function manyRequestsAreBounded() {
53+
// A peer that accepts the connection and then stops reading. Before the
54+
// session memory check, every request was queued regardless of the limit.
55+
const server = http2.createServer();
56+
server.on('connection', (socket) => setTimeout(() => socket.pause(), 30));
57+
server.on('stream', (stream) => stream.on('error', () => {}));
58+
server.listen(0);
59+
await once(server, 'listening');
60+
61+
const client = http2.connect(`http://localhost:${server.address().port}`, {
62+
maxSessionMemory: 1,
63+
});
64+
client.on('error', () => {});
65+
await once(client, 'connect');
66+
67+
const total = 30_000;
68+
let rejected = 0;
69+
for (let i = 0; i < total; i++) {
70+
const req = client.request({ ':path': `/${i}` });
71+
req.on('error', (err) => {
72+
if (err.code === 'ERR_HTTP2_STREAM_ERROR' &&
73+
err.message.includes('NGHTTP2_ENHANCE_YOUR_CALM')) {
74+
rejected++;
75+
}
76+
});
77+
req.end();
78+
}
79+
await new Promise(setImmediate);
80+
81+
assert.ok(rejected > 0, 'expected some requests to be rejected');
82+
// Without the check all of the requests (plus the connection preface) were
83+
// queued: the number of queued frames grew with the number submitted.
84+
assert.ok(client.state.outboundQueueSize < total,
85+
`queued ${client.state.outboundQueueSize} of ${total}`);
86+
assert.strictEqual(NGHTTP2_ENHANCE_YOUR_CALM, 11);
87+
88+
client.destroy();
89+
server.close();
90+
}
91+
92+
(async () => {
93+
await queuedUploadDoesNotBlockRequests();
94+
await manyRequestsAreBounded();
95+
})().then(common.mustCall());

0 commit comments

Comments
 (0)