Skip to content

Commit 8d45d5b

Browse files
inoway46aduh95
inoway46
authored and
aduh95
committed
http2: exclude queued data from response memory checks
Allow clients to receive responses when queued request data exceeds maxSessionMemory, while preserving limits on received headers. Fixes: #66739 Refs: #67113 Assisted-by: Codex Signed-off-by: inoway46 <inoueyuya416@gmail.com> PR-URL: #67114 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent 5991efa commit 8d45d5b

8 files changed

Lines changed: 338 additions & 10 deletions

‎doc/api/http2.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3354,7 +3354,11 @@ changes:
33543354
the current memory use of the header compression tables, header blocks
33553355
retained by open streams, current data queued to be sent, and
33563356
unacknowledged `PING` and `SETTINGS` frames are all counted towards the
3357-
current limit. **Default:** `10`.
3357+
current limit. For received headers on locally initiated streams, including
3358+
informational responses and trailers, queued outbound data across the session
3359+
is excluded from the memory check. Retained headers still count towards the
3360+
limit. This exception does not apply to server push streams.
3361+
**Default:** `10`.
33583362
* `maxHeaderListPairs` {number} Sets the maximum number of header entries.
33593363
This is similar to [`server.maxHeadersCount`][] or
33603364
[`request.maxHeadersCount`][] in the `node:http` module. The minimum value

‎src/node_http2.cc‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2782,9 +2782,11 @@ bool Http2Stream::AddHeader(nghttp2_rcbuf* name,
27822782

27832783
Http2Header header(env(), name, value, flags);
27842784
size_t length = header.length() + 32;
2785-
// A header can only be added if we have not exceeded the maximum number
2786-
// of headers and the session has memory available for it.
2787-
if (!session_->has_available_session_memory(length) ||
2785+
// Queued request bodies must not block response headers on local streams.
2786+
// Push streams have even IDs and retain the full session memory check.
2787+
const bool exclude_outbound =
2788+
session_->type() == NGHTTP2_SESSION_CLIENT && (id_ & 1) != 0;
2789+
if (!session_->has_available_session_memory(length, exclude_outbound) ||
27882790
current_headers_.size() == max_header_pairs_ ||
27892791
current_headers_length_ + length > max_header_length_) {
27902792
return false;
@@ -2895,12 +2897,13 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle,
28952897

28962898
void Http2Stream::IncrementAvailableOutboundLength(size_t amount) {
28972899
available_outbound_length_ += amount;
2898-
session_->IncrementCurrentSessionMemory(amount);
2900+
session_->IncrementQueuedOutboundMemory(amount);
28992901
}
29002902

29012903
void Http2Stream::DecrementAvailableOutboundLength(size_t amount) {
2904+
DCHECK_LE(amount, available_outbound_length_);
29022905
available_outbound_length_ -= amount;
2903-
session_->DecrementCurrentSessionMemory(amount);
2906+
session_->DecrementQueuedOutboundMemory(amount);
29042907
}
29052908

29062909

‎src/node_http2.h‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -811,6 +811,17 @@ class Http2Session : public AsyncWrap,
811811
void UpdateLocalCustomSettings(size_t count_,
812812
nghttp2_settings_entry* entries_);
813813

814+
void IncrementQueuedOutboundMemory(uint64_t amount) {
815+
queued_outbound_memory_ += amount;
816+
IncrementCurrentSessionMemory(amount);
817+
}
818+
819+
void DecrementQueuedOutboundMemory(uint64_t amount) {
820+
DCHECK_LE(amount, queued_outbound_memory_);
821+
queued_outbound_memory_ -= amount;
822+
DecrementCurrentSessionMemory(amount);
823+
}
824+
814825
// Tell our custom memory allocator that this rcbuf is independent of
815826
// this session now, and may outlive it.
816827
void StopTrackingRcbuf(nghttp2_rcbuf* buf);
@@ -824,9 +835,17 @@ class Http2Session : public AsyncWrap,
824835
return total;
825836
}
826837

827-
// Return true if current_session_memory + amount is less than the max
828-
bool has_available_session_memory(uint64_t amount) const {
829-
return current_session_memory() + amount <= max_session_memory_;
838+
// Headers on local client streams may exclude queued outbound DATA, but
839+
// retained headers and all other session memory still count towards the
840+
// limit.
841+
bool has_available_session_memory(uint64_t amount,
842+
bool exclude_outbound = false) const {
843+
uint64_t memory = current_session_memory();
844+
if (exclude_outbound) {
845+
DCHECK_LE(queued_outbound_memory_, memory);
846+
memory -= queued_outbound_memory_;
847+
}
848+
return memory + amount <= max_session_memory_;
830849
}
831850

832851
struct Statistics {
@@ -966,6 +985,8 @@ class Http2Session : public AsyncWrap,
966985
// The maximum amount of memory allocated for this session
967986
uint64_t max_session_memory_ = kDefaultMaxSessionMemory;
968987
uint64_t current_session_memory_ = 0;
988+
// Part of current_session_memory_: DATA not yet consumed by nghttp2.
989+
uint64_t queued_outbound_memory_ = 0;
969990
// The amount of memory allocated by nghttp2 internals
970991
uint64_t current_nghttp2_memory_ = 0;
971992

‎test/parallel/test-http2-max-session-memory-cancel.js‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,14 @@ const { once } = require('events');
1111
(async () => {
1212
// Keep the entire upload queued until it is canceled.
1313
const server = http2.createServer({ settings: { initialWindowSize: 0 } });
14+
let serverPush;
1415
server.on('stream', common.mustCall((stream, headers) => {
1516
stream.on('error', () => {});
1617
if (headers[':path'] === '/cancel') return;
18+
stream.pushStream({ ':path': '/push' }, common.mustSucceed((push) => {
19+
serverPush = push;
20+
push.on('error', () => {});
21+
}));
1722
stream.respond({ ':status': 200 });
1823
stream.end();
1924
}, 2));
@@ -35,7 +40,9 @@ const { once } = require('events');
3540
// Native write queue cleanup runs on an immediate after stream destruction.
3641
await new Promise(setImmediate);
3742

38-
// The canceled upload must no longer count towards maxSessionMemory.
43+
// Push admission uses the full memory check, so leaked outbound accounting
44+
// cannot be hidden by the DATA exception on local streams.
45+
const promised = once(client, 'stream');
3946
const req = client.request({ ':path': '/check' });
4047
const response = once(req, 'response');
4148
req.end();
@@ -44,6 +51,14 @@ const { once } = require('events');
4451
const ended = once(req, 'end');
4552
req.resume();
4653
await ended;
54+
const [push] = await promised;
55+
const pushedResponse = once(push, 'push');
56+
const pushedEnd = once(push, 'end');
57+
push.resume();
58+
serverPush.respond({ ':status': 200 });
59+
serverPush.end();
60+
const [[pushedHeaders]] = await Promise.all([pushedResponse, pushedEnd]);
61+
assert.strictEqual(pushedHeaders[':status'], 200);
4762
client.close();
4863
server.close();
4964
})().then(common.mustCall());

‎test/parallel/test-http2-max-session-memory-response-limits.js‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
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+
// Excluding queued DATA must preserve the cumulative header budget and
12+
// the independent header count and size limits.
13+
async function test(kind, bodySize = 4 * 1024 * 1024) {
14+
const sessions = new Set();
15+
const server = http2.createServer({ settings: { initialWindowSize: 0 } });
16+
server.on('session', (session) => sessions.add(session));
17+
server.on('stream', (stream) => {
18+
stream.on('error', () => {});
19+
switch (kind) {
20+
case 'repeated-informational':
21+
for (let i = 0; i < 80; i++) {
22+
stream.additionalHeaders({ ':status': 103,
23+
'x-hint': 'x'.repeat(16 * 1024) });
24+
}
25+
break;
26+
case 'pairs':
27+
stream.respond({ ':status': 413, 'a': '1', 'b': '2', 'c': '3', 'd': '4' });
28+
stream.end();
29+
return;
30+
case 'size':
31+
stream.respond({ ':status': 413, 'x-large': 'x'.repeat(2048) });
32+
stream.end();
33+
return;
34+
}
35+
stream.respond({ ':status': 413 });
36+
stream.end();
37+
});
38+
server.listen(0);
39+
await once(server, 'listening');
40+
const client = http2.connect(`http://localhost:${server.address().port}`, {
41+
maxSessionMemory: 1,
42+
maxHeaderListPairs: kind === 'pairs' ? 4 : 128,
43+
settings: { maxHeaderListSize: kind === 'size' ? 1024 : 65535 },
44+
});
45+
await Promise.all([
46+
once(client, 'remoteSettings'),
47+
once(client, 'localSettings'),
48+
]);
49+
const req = client.request({ ':method': 'POST' });
50+
const failure = once(req, 'error');
51+
let hints = 0;
52+
if (kind === 'repeated-informational') {
53+
req.on('headers', () => hints++);
54+
} else {
55+
req.on('headers', common.mustNotCall());
56+
}
57+
req.on('response', common.mustNotCall());
58+
req.on('trailers', common.mustNotCall());
59+
req.end(Buffer.alloc(bodySize));
60+
const [err] = await failure;
61+
assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_ERROR');
62+
if (kind === 'repeated-informational') {
63+
assert(hints > 0 && hints < 80);
64+
}
65+
client.destroy();
66+
for (const session of sessions) session.destroy();
67+
server.close();
68+
}
69+
70+
(async () => {
71+
await test('pairs');
72+
await test('size');
73+
await test('repeated-informational', 0);
74+
await test('repeated-informational');
75+
})().then(common.mustCall());

‎test/parallel/test-http2-max-session-memory-response-push.js‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
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+
(async () => {
12+
const sessions = new Set();
13+
let serverRequest;
14+
let serverPush;
15+
const server = http2.createServer({ settings: { initialWindowSize: 0 } });
16+
server.on('session', (session) => sessions.add(session));
17+
server.on('stream', (stream) => {
18+
serverRequest = stream;
19+
stream.on('error', () => {});
20+
stream.pushStream({ ':path': '/push' }, common.mustSucceed((push) => {
21+
serverPush = push;
22+
push.on('error', () => {});
23+
}));
24+
});
25+
server.listen(0);
26+
await once(server, 'listening');
27+
const client = http2.connect(`http://localhost:${server.address().port}`, {
28+
maxSessionMemory: 1,
29+
});
30+
await Promise.all([
31+
once(client, 'remoteSettings'),
32+
once(client, 'localSettings'),
33+
]);
34+
const promised = once(client, 'stream');
35+
const req = client.request({ ':method': 'POST' });
36+
const response = once(req, 'response');
37+
const [push] = await promised;
38+
push.on('push', common.mustNotCall());
39+
const failure = once(push, 'error');
40+
41+
// Accept the PUSH_PROMISE first, then exhaust the budget. Even an existing
42+
// pushed stream must not exclude queued DATA when receiving its response.
43+
req.end(Buffer.alloc(4 * 1024 * 1024));
44+
serverPush.respond({ ':status': 200 });
45+
serverPush.end();
46+
const [err] = await failure;
47+
assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_ERROR');
48+
49+
serverRequest.respond({ ':status': 413 });
50+
serverRequest.end();
51+
const [headers] = await response;
52+
assert.strictEqual(headers[':status'], 413);
53+
req.close();
54+
client.destroy();
55+
for (const session of sessions) session.destroy();
56+
server.close();
57+
})().then(common.mustCall());

‎test/parallel/test-http2-max-session-memory-response.js‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
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+
// Queued request bodies must not prevent responses on local streams.
12+
async function test(bodySize, {
13+
informational = 0,
14+
blocked = false,
15+
trailers = false,
16+
} = {}) {
17+
const server = http2.createServer({
18+
settings: { initialWindowSize: blocked ? 0 : 65535 },
19+
});
20+
const sessions = new Set();
21+
server.on('session', (session) => sessions.add(session));
22+
server.on('stream', (stream, headers) => {
23+
stream.on('error', () => {});
24+
if (headers[':path'] === '/reuse') {
25+
stream.additionalHeaders({ ':status': 103 });
26+
stream.respond({ ':status': 200 });
27+
stream.end();
28+
} else {
29+
if (informational) stream.additionalHeaders({ ':status': informational });
30+
stream.respond({ ':status': 413 }, { waitForTrailers: trailers });
31+
if (trailers) {
32+
stream.on('wantTrailers', () => stream.sendTrailers({ 'x-trailer': 'x' }));
33+
}
34+
stream.end();
35+
}
36+
});
37+
server.listen(0);
38+
await once(server, 'listening');
39+
40+
const client = http2.connect(`http://localhost:${server.address().port}`, {
41+
maxSessionMemory: 1,
42+
});
43+
await Promise.all([
44+
once(client, 'remoteSettings'),
45+
once(client, 'localSettings'),
46+
]);
47+
const req = client.request({ ':method': 'POST' });
48+
const response = once(req, 'response');
49+
const ended = once(req, 'end');
50+
const closed = once(req, 'close');
51+
req.on('headers', informational ? common.mustCall((headers) => {
52+
assert.strictEqual(headers[':status'], informational);
53+
}) : common.mustNotCall());
54+
req.on('trailers', trailers ? common.mustCall((headers) => {
55+
assert.strictEqual(headers['x-trailer'], 'x');
56+
}) : common.mustNotCall());
57+
req.resume();
58+
// Fill the write queue before any response headers can arrive.
59+
req.end(Buffer.alloc(bodySize));
60+
const [[headers]] = await Promise.all([response, ended]);
61+
assert.strictEqual(headers[':status'], 413);
62+
req.destroy();
63+
await closed;
64+
// The session must remain usable after canceling the upload.
65+
const reuse = client.request({ ':path': '/reuse' });
66+
reuse.on('headers', common.mustCall());
67+
const reusedResponse = once(reuse, 'response');
68+
reuse.end();
69+
const [reusedHeaders] = await reusedResponse;
70+
assert.strictEqual(reusedHeaders[':status'], 200);
71+
const reusedClose = once(reuse, 'close');
72+
reuse.close();
73+
await reusedClose;
74+
client.destroy();
75+
for (const session of sessions) session.destroy();
76+
server.close();
77+
}
78+
79+
(async () => {
80+
const bodySize = 4 * 1024 * 1024;
81+
await test(bodySize);
82+
await test(bodySize, { blocked: true });
83+
await test(bodySize, { informational: 100, blocked: true });
84+
await test(bodySize, { informational: 103, blocked: true });
85+
await test(bodySize, { trailers: true, blocked: true });
86+
await test(1);
87+
})().then(common.mustCall());

0 commit comments

Comments
 (0)