Skip to content

Commit e766c3d

Browse files
mcollinaaduh95
authored andcommitted
http2: charge inbound DATA retained by streams to maxSessionMemory
Inbound DATA frames delivered to JS while a stream is paused are retained in the stream's readable buffer until the application reads them. These were not counted against the session's maxSessionMemory, so a peer could send many tiny DATA frames on a stream the application is not draining and make the session retain an unbounded amount of memory, in proportion to the number of frames rather than the bytes sent. Charge each such DATA chunk (plus a per-chunk overhead comparable to the Buffer view bookkeeping) against maxSessionMemory, and release it when the stream resumes reading (ReadStart) or is destroyed (RemoveStream). Add connection-level backpressure so the session stops reading from the socket once it can no longer afford the inbound queued data, resuming when the application consumes it. Refs: https://hackerone.com/reports/4089663 Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs-private/node-private#1000 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> CVE-ID: CVE-2026-123868
1 parent 5e1dad2 commit e766c3d

4 files changed

Lines changed: 203 additions & 11 deletions

File tree

‎doc/api/http2.md‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2928,9 +2928,12 @@ changes:
29282928
limit to be exceeded, but new `Http2Stream` instances will be rejected
29292929
while this limit is exceeded. The current number of `Http2Stream` sessions,
29302930
the current memory use of the header compression tables, header blocks
2931-
retained by open streams, current data queued to be sent, and
2932-
unacknowledged `PING` and `SETTINGS` frames are all counted towards the
2933-
current limit. **Default:** `10`.
2931+
retained by open streams, current data queued to be sent, inbound DATA
2932+
frames retained by undrained streams, and unacknowledged `PING` and
2933+
`SETTINGS` frames are all counted towards the current limit. When the limit
2934+
is exceeded the session stops reading from the socket (applying
2935+
backpressure) until the application consumes the queued data. **Default:**
2936+
`10`.
29342937
* `maxHeaderListPairs` {number} Sets the maximum number of header entries.
29352938
This is similar to [`server.maxHeadersCount`][] or
29362939
[`request.maxHeadersCount`][] in the `node:http` module. The minimum value
@@ -3160,9 +3163,12 @@ changes:
31603163
limit to be exceeded, but new `Http2Stream` instances will be rejected
31613164
while this limit is exceeded. The current number of `Http2Stream` sessions,
31623165
the current memory use of the header compression tables, header blocks
3163-
retained by open streams, current data queued to be sent, and
3164-
unacknowledged `PING` and `SETTINGS` frames are all counted towards the
3165-
current limit. **Default:** `10`.
3166+
retained by open streams, current data queued to be sent, inbound DATA
3167+
frames retained by undrained streams, and unacknowledged `PING`
3168+
and `SETTINGS` frames are all counted towards the current limit. When the
3169+
limit is exceeded the session stops reading from the socket (applying
3170+
backpressure) until the application consumes the queued data. **Default:**
3171+
`10`.
31663172
* `maxHeaderListPairs` {number} Sets the maximum number of header entries.
31673173
This is similar to [`server.maxHeadersCount`][] or
31683174
[`request.maxHeadersCount`][] in the `node:http` module. The minimum value
@@ -3352,9 +3358,12 @@ changes:
33523358
limit to be exceeded, but new `Http2Stream` instances will be rejected
33533359
while this limit is exceeded. The current number of `Http2Stream` sessions,
33543360
the current memory use of the header compression tables, header blocks
3355-
retained by open streams, current data queued to be sent, and
3361+
retained by open streams, current data queued to be sent, inbound DATA
3362+
frames retained by undrained streams, and
33563363
unacknowledged `PING` and `SETTINGS` frames are all counted towards the
3357-
current limit. For received headers on locally initiated streams, including
3364+
current limit. When the limit is exceeded the session stops reading from the
3365+
socket (applying backpressure) until the application consumes the queued data.
3366+
For received headers on locally initiated streams, including
33583367
informational responses and trailers, queued outbound data across the session
33593368
is excluded from the memory check. Retained headers still count towards the
33603369
limit. This exception does not apply to server push streams.

‎src/node_http2.cc‎

Lines changed: 62 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,10 @@ Http2Options::Http2Options(Http2State* http2_state, SessionType type) {
205205
// this is a credit based system. Existing streams may cause the limit
206206
// to be temporarily exceeded but once over the limit, new streams cannot
207207
// created.
208+
// Inbound DATA that is queued in the readable buffer of an undrained stream
209+
// also counts against this limit, and when the limit is reached the session
210+
// stops reading from the socket (applying backpressure) until the
211+
// application consumes the data.
208212
// Important: The maxSessionMemory option in javascript is expressed in
209213
// terms of MB increments (i.e. the value 1 == 1 MB)
210214
if (flags & (1 << IDX_OPTIONS_MAX_SESSION_MEMORY))
@@ -965,8 +969,18 @@ BaseObjectPtr<Http2Stream> Http2Session::RemoveStream(int32_t id) {
965969
DecrementCurrentSessionMemory(stream->retained_headers_length_);
966970
stream->retained_headers_length_ = 0;
967971
}
972+
if (stream->inbound_consumed_data_while_paused_memory_ > 0) {
973+
DecrementCurrentSessionMemory(
974+
stream->inbound_consumed_data_while_paused_memory_);
975+
inbound_queued_data_memory_ -=
976+
stream->inbound_consumed_data_while_paused_memory_;
977+
stream->inbound_consumed_data_while_paused_memory_ = 0;
978+
}
968979
DecrementCurrentSessionMemory(sizeof(*stream));
969980
}
981+
// Removing the stream may have freed inbound data memory that was keeping
982+
// the session from reading; resume if it now can make progress.
983+
MaybeStartReading();
970984
return stream;
971985
}
972986

@@ -1477,11 +1491,23 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle,
14771491
// If the stream owner (e.g. the JS Http2Stream) wants more data, just
14781492
// tell nghttp2 that all data has been consumed. Otherwise, defer until
14791493
// more data is being requested.
1480-
if (stream->is_reading())
1494+
if (stream->is_reading()) {
14811495
nghttp2_session_consume_stream(handle, id, avail);
1482-
else
1496+
} else {
14831497
stream->inbound_consumed_data_while_paused_ += avail;
14841498

1499+
// The Buffer handed to JS may stay alive in the readable queue for as
1500+
// long as the stream remains paused. Charge it (including a per-chunk
1501+
// overhead for the Buffer view) against maxSessionMemory so that a peer
1502+
// cannot make the session retain an unbounded amount of memory by
1503+
// sending many tiny DATA frames.
1504+
uint64_t memory =
1505+
static_cast<uint64_t>(avail) + kHttp2SessionDataChunkOverhead;
1506+
stream->inbound_consumed_data_while_paused_memory_ += memory;
1507+
session->inbound_queued_data_memory_ += memory;
1508+
session->IncrementCurrentSessionMemory(memory);
1509+
}
1510+
14851511
// If we have a gathered a lot of data for output, try sending it now.
14861512
if (session->outgoing_length_ > 4096 ||
14871513
stream->available_outbound_length_ > 4096) {
@@ -1931,12 +1957,30 @@ void Http2Session::MaybeStopReading() {
19311957
if (is_reading_stopped() || is_closing()) return;
19321958
int want_read = nghttp2_session_want_read(session_.get());
19331959
Debug(this, "wants read? %d", want_read);
1934-
if (want_read == 0) {
1960+
// Stop reading if nghttp2 does not want more data, a write is in progress,
1961+
// or the session is already holding more inbound DATA than its memory budget
1962+
// allows. The last condition bounds the amount of memory a peer can make the
1963+
// session retain by sending many small DATA frames on undrained streams.
1964+
if (want_read == 0 || inbound_queued_data_memory_ >= max_session_memory_) {
19351965
set_reading_stopped();
19361966
stream_->ReadStop();
19371967
}
19381968
}
19391969

1970+
void Http2Session::MaybeStartReading() {
1971+
// Restart reading once the session can make progress: nghttp2 wants more
1972+
// data, no write is in flight, and the inbound DATA queued while streams
1973+
// were paused no longer exceeds the session memory budget.
1974+
if (!is_reading_stopped() || is_closing() || is_destroyed()) return;
1975+
int want_read = nghttp2_session_want_read(session_.get());
1976+
Debug(this, "want read? %d", want_read);
1977+
if (want_read != 0 && !is_write_in_progress() &&
1978+
inbound_queued_data_memory_ < max_session_memory_) {
1979+
set_reading_stopped(false);
1980+
stream_->ReadStart();
1981+
}
1982+
}
1983+
19401984
// Unset the sending state, finish up all current writes, and reset
19411985
// storage for data and metadata that was associated with these writes.
19421986
void Http2Session::ClearOutgoing(int status) {
@@ -2726,6 +2770,21 @@ int Http2Stream::ReadStart() {
27262770
inbound_consumed_data_while_paused_);
27272771
inbound_consumed_data_while_paused_ = 0;
27282772

2773+
// The data that was queued while paused is now being consumed by the
2774+
// application, so release the memory charged against maxSessionMemory.
2775+
if (inbound_consumed_data_while_paused_memory_ > 0) {
2776+
session_->DecrementCurrentSessionMemory(
2777+
inbound_consumed_data_while_paused_memory_);
2778+
session_->DecrementInboundQueuedDataMemory(
2779+
inbound_consumed_data_while_paused_memory_);
2780+
inbound_consumed_data_while_paused_memory_ = 0;
2781+
}
2782+
2783+
// The session may have stopped reading because the memory limit was
2784+
// reached; now that the application is consuming data, it may be able to
2785+
// make progress again.
2786+
session_->MaybeStartReading();
2787+
27292788
return 0;
27302789
}
27312790

‎src/node_http2.h‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,14 @@ constexpr size_t kDefaultMaxSettings = 10;
3939
// Default maximum total memory cap for Http2Session.
4040
constexpr uint64_t kDefaultMaxSessionMemory = 10000000;
4141

42+
// Per-DATA-chunk bookkeeping overhead charged against maxSessionMemory. DATA
43+
// frames delivered to JS are turned into Buffer views that stay alive in the
44+
// readable queue while a stream is paused. The overhead accounts for the
45+
// per-chunk view (and related) bookkeeping that is not captured by the payload
46+
// length alone, so that a peer sending many tiny DATA frames cannot make a
47+
// session retain an unbounded amount of memory.
48+
constexpr size_t kHttp2SessionDataChunkOverhead = 128;
49+
4250
// Default connection-level flow control window (32MB) to improve throughput
4351
// on high-latency connections. See https://github.com/nodejs/node/issues/38426
4452
constexpr int32_t kDefaultConnectionWindowSize = 33554432;
@@ -556,6 +564,12 @@ class Http2Stream : public AsyncWrap,
556564
// backpressure handling.
557565
size_t inbound_consumed_data_while_paused_ = 0;
558566

567+
// The amount of memory charged against maxSessionMemory_ for DATA frames
568+
// that were handed to JS while the stream was paused and therefore may still
569+
// be retained in the readable queue. Released when the stream resumes
570+
// reading (ReadStart) or is destroyed.
571+
uint64_t inbound_consumed_data_while_paused_memory_ = 0;
572+
559573
// Outbound Data... This is the data written by the JS layer that is
560574
// waiting to be written out to the socket.
561575
std::queue<NgHttp2StreamWrite> queue_;
@@ -722,6 +736,10 @@ class Http2Session : public AsyncWrap,
722736
// Stop reading if nghttp2 doesn't want to anymore.
723737
void MaybeStopReading();
724738

739+
// Restart reading if the session was stopped because of the memory limit and
740+
// can now make progress.
741+
void MaybeStartReading();
742+
725743
// Returns pointer to the stream, or nullptr if stream does not exist
726744
BaseObjectPtr<Http2Stream> FindStream(int32_t id);
727745

@@ -815,6 +833,14 @@ class Http2Session : public AsyncWrap,
815833
current_session_memory_ -= amount;
816834
}
817835

836+
// Called by a stream when it stops holding inbound DATA while paused, to
837+
// keep inbound_queued_data_memory_ (which drives connection-level
838+
// backpressure) in sync. The caller is responsible for also calling
839+
// DecrementCurrentSessionMemory for the same amount.
840+
void DecrementInboundQueuedDataMemory(uint64_t amount) {
841+
inbound_queued_data_memory_ -= amount;
842+
}
843+
818844
void UpdateLocalCustomSettings(size_t count_,
819845
nghttp2_settings_entry* entries_);
820846

@@ -994,6 +1020,11 @@ class Http2Session : public AsyncWrap,
9941020
uint64_t current_session_memory_ = 0;
9951021
// Part of current_session_memory_: DATA not yet consumed by nghttp2.
9961022
uint64_t queued_outbound_memory_ = 0;
1023+
// The portion of current_session_memory_ that is held by inbound DATA frames
1024+
// queued in stream readable buffers while the streams are paused. Used to
1025+
// apply connection-level backpressure (stop reading) once the session cannot
1026+
// afford to buffer more.
1027+
uint64_t inbound_queued_data_memory_ = 0;
9971028
// The amount of memory allocated by nghttp2 internals
9981029
uint64_t current_nghttp2_memory_ = 0;
9991030

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
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 net = require('net');
10+
11+
// Regression test: inbound DATA frames handed to JS on an undrained stream must
12+
// count against maxSessionMemory. Before the fix a peer could send many tiny
13+
// DATA frames on a stream the application never reads, and the session would
14+
// keep reading (and buffering) them. Because each frame becomes a Buffer view in
15+
// the readable queue, memory grew in proportion to the *number* of frames rather
16+
// than the bytes sent, so a single stream could make the process retain hundreds
17+
// of MB. With the fix the session charges the queued data against
18+
// maxSessionMemory and stops reading once it can no longer afford to buffer
19+
// more, keeping the readable buffer bounded.
20+
const maxSessionMemory = 1;
21+
const chunkSize = 1;
22+
const totalFrames = 200000;
23+
24+
let peakReadableLength = 0;
25+
26+
function frame(type, flags, id, payload) {
27+
const header = Buffer.alloc(9);
28+
header.writeUIntBE(payload.length, 0, 3);
29+
header[3] = type;
30+
header[4] = flags;
31+
header.writeUInt32BE(id, 5);
32+
return Buffer.concat([header, payload]);
33+
}
34+
35+
let session = null;
36+
let probe = null;
37+
38+
const server = http2.createServer({ maxSessionMemory });
39+
server.on('session', (s) => {
40+
session = s;
41+
s.on('error', () => {});
42+
});
43+
server.on('stream', (stream) => {
44+
stream.on('error', () => {});
45+
// The application never reads the request body, so the stream stays
46+
// undrained. Record the largest amount of data the readable buffer ever holds;
47+
// with the fix that stays bounded by maxSessionMemory instead of growing to
48+
// match the number of frames the peer sends.
49+
probe = setInterval(() => {
50+
peakReadableLength = Math.max(peakReadableLength, stream.readableLength);
51+
}, 10);
52+
});
53+
54+
server.listen(0, common.mustCall(() => {
55+
const sock = net.connect(server.address().port, '127.0.0.1');
56+
sock.on('error', () => {});
57+
58+
sock.on('connect', () => {
59+
sock.write(Buffer.from('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'));
60+
// Client SETTINGS.
61+
sock.write(frame(4, 0, 0, Buffer.alloc(0)));
62+
});
63+
64+
let settingsAcked = false;
65+
sock.on('data', (chunk) => {
66+
if (!settingsAcked && chunk.length >= 9 && chunk[3] === 4) {
67+
settingsAcked = true;
68+
// ACK the server's SETTINGS.
69+
sock.write(Buffer.from([0, 0, 0, 4, 1, 0, 0, 0, 0]));
70+
// Open one stream (END_HEADERS) and spray 1-byte DATA frames at it.
71+
const hpack = Buffer.from([0x83, 0x86, 0x84, 0x01, 0x01, 0x61]);
72+
sock.write(frame(1, 4, 1, hpack));
73+
const payload = Buffer.alloc(chunkSize, 0x61);
74+
const parts = [];
75+
for (let i = 0; i < totalFrames; i++) {
76+
parts.push(frame(0, 0, 1, payload));
77+
}
78+
sock.write(Buffer.concat(parts));
79+
}
80+
});
81+
82+
setTimeout(common.mustCall(() => {
83+
if (probe) clearInterval(probe);
84+
sock.destroy();
85+
if (session) session.destroy();
86+
server.close();
87+
// The session must not let one undrained stream buffer every frame it was
88+
// sent. Before the fix the readable buffer grew to totalFrames; with the fix
89+
// the backpressure keeps it bounded well below half of that.
90+
assert.ok(peakReadableLength < totalFrames / 2,
91+
`expected backpressure to bound readableLength, got ${peakReadableLength}`);
92+
}), 4000);
93+
}));

0 commit comments

Comments
 (0)