Skip to content

Commit bfe0463

Browse files
ggreenwayzmiklank
ggreenway
authored and
zmiklank
committed
oghttp2: fix window buffer leak
Backport fix for #40085 Signed-off-by: Greg Greenway <[email protected]>
1 parent 9d9191d commit bfe0463

File tree

2 files changed

+229
-0
lines changed

2 files changed

+229
-0
lines changed

‎bazel/external/quiche.patch‎

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
commit 6c726cca04b68c5593fbded31fadb001771844ef
2+
Author: danzh <[email protected]>
3+
Date: Wed Jul 9 08:28:21 2025 -0700
4+
5+
Patch a fix from https://github.com/villainb-dg for the issue of leaking connection flow control window when a HTTP2 stream receives a DATA frame after being closed (reset, etc).
6+
7+
When a stream receives a DATA frame while it is already closed, the data is counted against the connection flow control window, but is never marked as consumed.
8+
Fix: Mark the data as consumed when the received DATA frame is on a reset or invalid stream.
9+
10+
Reported QUICHE issue https://github.com/google/quiche/issues/91
11+
Reported Envoy issue https://github.com/envoyproxy/envoy/issues/40085
12+
Proposed external fix: https://github.com/google/quiche/pull/92
13+
14+
PiperOrigin-RevId: 781064590
15+
16+
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc
17+
index c25b2917b..9b1afb22b 100644
18+
--- a/quiche/http2/adapter/oghttp2_session.cc
19+
+++ b/quiche/http2/adapter/oghttp2_session.cc
20+
@@ -1152,11 +1152,15 @@ void OgHttp2Session::OnDataFrameHeader(spdy::SpdyStreamId stream_id,
21+
22+
void OgHttp2Session::OnStreamFrameData(spdy::SpdyStreamId stream_id,
23+
const char* data, size_t len) {
24+
- // Count the data against flow control, even if the stream is unknown.
25+
+ // Count the data against flow control, even if the stream is unknown, so that
26+
+ // the connection flow control window is in sync with peer's.
27+
MarkDataBuffered(stream_id, len);
28+
29+
auto iter = stream_map_.find(stream_id);
30+
if (iter == stream_map_.end()) {
31+
+ // Mark the data consumed immediately as we are dropping them. This will
32+
+ // allow the connection flow control window to shift.
33+
+ Consume(stream_id, len);
34+
return;
35+
}
36+
// Validate against the content-length if it exists.
37+
@@ -1171,6 +1175,9 @@ void OgHttp2Session::OnStreamFrameData(spdy::SpdyStreamId stream_id,
38+
if (streams_reset_.contains(stream_id)) {
39+
// If the stream was unknown due to a protocol error, the visitor was
40+
// informed in OnDataFrameHeader().
41+
+ // Mark the data consumed immediately as we are dropping them. This will
42+
+ // allow the connection flow control window to shift.
43+
+ Consume(stream_id, len);
44+
return;
45+
}
46+
47+
diff --git a/quiche/http2/adapter/oghttp2_session_test.cc b/quiche/http2/adapter/oghttp2_session_test.cc
48+
index 34a387bec..4afcf1a3d 100644
49+
--- a/quiche/http2/adapter/oghttp2_session_test.cc
50+
+++ b/quiche/http2/adapter/oghttp2_session_test.cc
51+
@@ -1217,6 +1217,175 @@ TEST(OgHttp2SessionTest, ServerClosesStreamDuringOnEndStream) {
52+
EXPECT_EQ(result, frames.size());
53+
}
54+
55+
+TEST(OgHttp2SessionTest, ResetStreamRaceWithIncomingData) {
56+
+ TestVisitor visitor;
57+
+ OgHttp2Session::Options options;
58+
+ options.perspective = Perspective::kServer;
59+
+ OgHttp2Session session(visitor, options);
60+
+
61+
+ const std::string frames = TestFrameSequence()
62+
+ .ClientPreface()
63+
+ .Headers(1,
64+
+ {{":method", "POST"},
65+
+ {":scheme", "https"},
66+
+ {":authority", "example.com"},
67+
+ {":path", "/"}},
68+
+ /*fin=*/false)
69+
+ .Data(1, "Request body", false)
70+
+ .Serialize();
71+
+ testing::InSequence s;
72+
+
73+
+ // Client preface (empty SETTINGS)
74+
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
75+
+ EXPECT_CALL(visitor, OnSettingsStart());
76+
+ EXPECT_CALL(visitor, OnSettingsEnd());
77+
+ // Stream 1
78+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x4));
79+
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
80+
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "POST"));
81+
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https"));
82+
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com"));
83+
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/"));
84+
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
85+
+
86+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0x0));
87+
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
88+
+ EXPECT_CALL(visitor, OnDataForStream(1, "Request body"))
89+
+ .WillOnce(testing::InvokeWithoutArgs([&session]() {
90+
+ session.Consume(1, 12);
91+
+ return true;
92+
+ }));
93+
+
94+
+ session.ProcessBytes(frames);
95+
+
96+
+ EXPECT_TRUE(session.want_write());
97+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
98+
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
99+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1));
100+
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0));
101+
+ int result1 = session.Send();
102+
+ EXPECT_EQ(0, result1);
103+
+ absl::string_view serialized1 = visitor.data();
104+
+ EXPECT_THAT(serialized1,
105+
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS}));
106+
+ EXPECT_FALSE(session.want_write());
107+
+
108+
+ EXPECT_LT(session.GetReceiveWindowSize(), kInitialFlowControlWindowSize);
109+
+
110+
+ // Reset the stream and receive more data on this stream.
111+
+ session.EnqueueFrame(std::make_unique<spdy::SpdyRstStreamIR>(
112+
+ 1, spdy::ERROR_CODE_PROTOCOL_ERROR));
113+
+ const std::string more_frames =
114+
+ TestFrameSequence()
115+
+ .Data(1, std::string(16 * 1024, 'x'), false)
116+
+ .Data(1, std::string(16 * 1024, 'y'), false)
117+
+ .Serialize();
118+
+ // These bytes are counted against the connection flow control window but
119+
+ // should be dropped right away and considerred as consumed.
120+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, _)).Times(0);
121+
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _)).Times(0);
122+
+ EXPECT_CALL(visitor, OnDataForStream(1, _)).Times(0);
123+
+
124+
+ session.ProcessBytes(more_frames);
125+
+ EXPECT_TRUE(session.want_write());
126+
+
127+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0));
128+
+ EXPECT_CALL(visitor, OnFrameSent(RST_STREAM, 1, _, 0x0, 1));
129+
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR));
130+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 0, _, 0x0));
131+
+ EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 0, _, 0x0, 0));
132+
+ int result2 = session.Send();
133+
+ EXPECT_EQ(0, result2);
134+
+ absl::string_view serialized2 = visitor.data();
135+
+ serialized2.remove_prefix(serialized1.size());
136+
+ EXPECT_THAT(serialized2, EqualsFrames({SpdyFrameType::RST_STREAM,
137+
+ SpdyFrameType::WINDOW_UPDATE}));
138+
+ EXPECT_EQ(session.GetReceiveWindowSize(), kInitialFlowControlWindowSize);
139+
+}
140+
+
141+
+TEST(OgHttp2SessionTest, ResetAndCloseStreamRaceWithIncomingData) {
142+
+ TestVisitor visitor;
143+
+ OgHttp2Session::Options options;
144+
+ options.perspective = Perspective::kServer;
145+
+ OgHttp2Session session(visitor, options);
146+
+
147+
+ const std::string frames = TestFrameSequence()
148+
+ .ClientPreface()
149+
+ .Headers(1,
150+
+ {{":method", "POST"},
151+
+ {":scheme", "https"},
152+
+ {":authority", "example.com"},
153+
+ {":path", "/"}},
154+
+ /*fin=*/false)
155+
+ .Data(1, "Request body", false)
156+
+ .Serialize();
157+
+ testing::InSequence s;
158+
+
159+
+ // Client preface (empty SETTINGS)
160+
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
161+
+ EXPECT_CALL(visitor, OnSettingsStart());
162+
+ EXPECT_CALL(visitor, OnSettingsEnd());
163+
+ // Stream 1
164+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x4));
165+
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
166+
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "POST"));
167+
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https"));
168+
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com"));
169+
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/"));
170+
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
171+
+
172+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0x0));
173+
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
174+
+ EXPECT_CALL(visitor, OnDataForStream(1, "Request body"))
175+
+ .WillOnce(testing::InvokeWithoutArgs([&session]() {
176+
+ session.Consume(1, 12);
177+
+ return true;
178+
+ }));
179+
+
180+
+ session.ProcessBytes(frames);
181+
+
182+
+ EXPECT_TRUE(session.want_write());
183+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
184+
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
185+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1));
186+
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0));
187+
+ int result1 = session.Send();
188+
+ EXPECT_EQ(0, result1);
189+
+ absl::string_view serialized1 = visitor.data();
190+
+ EXPECT_THAT(serialized1,
191+
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS}));
192+
+ EXPECT_FALSE(session.want_write());
193+
+
194+
+ EXPECT_LT(session.GetReceiveWindowSize(), kInitialFlowControlWindowSize);
195+
+
196+
+ // Reset the stream and receive more data on this stream.
197+
+ session.EnqueueFrame(std::make_unique<spdy::SpdyRstStreamIR>(
198+
+ 1, spdy::ERROR_CODE_PROTOCOL_ERROR));
199+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0));
200+
+ EXPECT_CALL(visitor, OnFrameSent(RST_STREAM, 1, _, 0x0, 1));
201+
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR));
202+
+ EXPECT_EQ(0, session.Send());
203+
+
204+
+ const std::string more_frames =
205+
+ TestFrameSequence()
206+
+ .Data(1, std::string(16 * 1024, 'x'), false)
207+
+ .Data(1, std::string(16 * 1024, 'y'), false)
208+
+ .Serialize();
209+
+ // These bytes are counted against the connection flow control window but
210+
+ // should be dropped right away and considered as consumed.
211+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, _)).Times(2);
212+
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _)).Times(0);
213+
+ EXPECT_CALL(visitor, OnDataForStream(1, _)).Times(0);
214+
+
215+
+ session.ProcessBytes(more_frames);
216+
+ EXPECT_TRUE(session.want_write());
217+
+
218+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 0, _, 0x0));
219+
+ EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 0, _, 0x0, 0));
220+
+ EXPECT_EQ(0, session.Send());
221+
+ EXPECT_EQ(session.GetReceiveWindowSize(), kInitialFlowControlWindowSize);
222+
+}
223+
+
224+
} // namespace test
225+
} // namespace adapter
226+
} // namespace http2

‎changelogs/current.yaml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ bug_fixes:
1515
getting the correct value. This could be triggered with a ``tcp_proxy`` access log configured to emit a log upon connection
1616
establishment if the log contains fields of the the TLS peer certificate. Then a later use of the data, such as the network RBAC
1717
filter validating a peer certificate SAN, may incorrectly fail due to the empty cached value.
18+
- area: http2
19+
change: |
20+
Fixed an issue where http/2 connections using the default codec of ``oghttp2`` could get stuck due to a window buffer leak.
1821
1922
removed_config_or_runtime:
2023
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

0 commit comments

Comments
 (0)