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).
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.
Fix: Mark the data as consumed when the received DATA frame is on a reset or invalid stream.
Reported QUICHE issue https://github.com/google/quiche/issues/91
Reported Envoy issue https://github.com/envoyproxy/envoy/issues/40085
Proposed external fix: https://github.com/google/quiche/pull/92
PiperOrigin-RevId: 781064590
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc
index c25b291..9b1afb2 100644
--- a/quiche/http2/adapter/oghttp2_session.cc
+++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -1152,11 +1152,15 @@
void OgHttp2Session::OnStreamFrameData(spdy::SpdyStreamId stream_id,
const char* data, size_t len) {
- // Count the data against flow control, even if the stream is unknown.
+ // Count the data against flow control, even if the stream is unknown, so that
+ // the connection flow control window is in sync with peer's.
MarkDataBuffered(stream_id, len);
auto iter = stream_map_.find(stream_id);
if (iter == stream_map_.end()) {
+ // Mark the data consumed immediately as we are dropping them. This will
+ // allow the connection flow control window to shift.
+ Consume(stream_id, len);
return;
}
// Validate against the content-length if it exists.
@@ -1171,6 +1175,9 @@
if (streams_reset_.contains(stream_id)) {
// If the stream was unknown due to a protocol error, the visitor was
// informed in OnDataFrameHeader().
+ // Mark the data consumed immediately as we are dropping them. This will
+ // allow the connection flow control window to shift.
+ Consume(stream_id, len);
return;
}
diff --git a/quiche/http2/adapter/oghttp2_session_test.cc b/quiche/http2/adapter/oghttp2_session_test.cc
index 34a387b..4afcf1a 100644
--- a/quiche/http2/adapter/oghttp2_session_test.cc
+++ b/quiche/http2/adapter/oghttp2_session_test.cc
@@ -1217,6 +1217,175 @@
EXPECT_EQ(result, frames.size());
}
+TEST(OgHttp2SessionTest, ResetStreamRaceWithIncomingData) {
+ TestVisitor visitor;
+ OgHttp2Session::Options options;
+ options.perspective = Perspective::kServer;
+ OgHttp2Session session(visitor, options);
+
+ const std::string frames = TestFrameSequence()
+ .ClientPreface()
+ .Headers(1,
+ {{":method", "POST"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"}},
+ /*fin=*/false)
+ .Data(1, "Request body", false)
+ .Serialize();
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+ // Stream 1
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x4));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "POST"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/"));
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0x0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
+ EXPECT_CALL(visitor, OnDataForStream(1, "Request body"))
+ .WillOnce(testing::InvokeWithoutArgs([&session]() {
+ session.Consume(1, 12);
+ return true;
+ }));
+
+ session.ProcessBytes(frames);
+
+ EXPECT_TRUE(session.want_write());
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0));
+ int result1 = session.Send();
+ EXPECT_EQ(0, result1);
+ absl::string_view serialized1 = visitor.data();
+ EXPECT_THAT(serialized1,
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS}));
+ EXPECT_FALSE(session.want_write());
+
+ EXPECT_LT(session.GetReceiveWindowSize(), kInitialFlowControlWindowSize);
+
+ // Reset the stream and receive more data on this stream.
+ session.EnqueueFrame(std::make_unique<spdy::SpdyRstStreamIR>(
+ 1, spdy::ERROR_CODE_PROTOCOL_ERROR));
+ const std::string more_frames =
+ TestFrameSequence()
+ .Data(1, std::string(16 * 1024, 'x'), false)
+ .Data(1, std::string(16 * 1024, 'y'), false)
+ .Serialize();
+ // These bytes are counted against the connection flow control window but
+ // should be dropped right away and considerred as consumed.
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, _)).Times(0);
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _)).Times(0);
+ EXPECT_CALL(visitor, OnDataForStream(1, _)).Times(0);
+
+ session.ProcessBytes(more_frames);
+ EXPECT_TRUE(session.want_write());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(RST_STREAM, 1, _, 0x0, 1));
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 0, _, 0x0, 0));
+ int result2 = session.Send();
+ EXPECT_EQ(0, result2);
+ absl::string_view serialized2 = visitor.data();
+ serialized2.remove_prefix(serialized1.size());
+ EXPECT_THAT(serialized2, EqualsFrames({SpdyFrameType::RST_STREAM,
+ SpdyFrameType::WINDOW_UPDATE}));
+ EXPECT_EQ(session.GetReceiveWindowSize(), kInitialFlowControlWindowSize);
+}
+
+TEST(OgHttp2SessionTest, ResetAndCloseStreamRaceWithIncomingData) {
+ TestVisitor visitor;
+ OgHttp2Session::Options options;
+ options.perspective = Perspective::kServer;
+ OgHttp2Session session(visitor, options);
+
+ const std::string frames = TestFrameSequence()
+ .ClientPreface()
+ .Headers(1,
+ {{":method", "POST"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"}},
+ /*fin=*/false)
+ .Data(1, "Request body", false)
+ .Serialize();
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+ // Stream 1
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x4));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "POST"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/"));
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0x0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
+ EXPECT_CALL(visitor, OnDataForStream(1, "Request body"))
+ .WillOnce(testing::InvokeWithoutArgs([&session]() {
+ session.Consume(1, 12);
+ return true;
+ }));
+
+ session.ProcessBytes(frames);
+
+ EXPECT_TRUE(session.want_write());
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0));
+ int result1 = session.Send();
+ EXPECT_EQ(0, result1);
+ absl::string_view serialized1 = visitor.data();
+ EXPECT_THAT(serialized1,
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS}));
+ EXPECT_FALSE(session.want_write());
+
+ EXPECT_LT(session.GetReceiveWindowSize(), kInitialFlowControlWindowSize);
+
+ // Reset the stream and receive more data on this stream.
+ session.EnqueueFrame(std::make_unique<spdy::SpdyRstStreamIR>(
+ 1, spdy::ERROR_CODE_PROTOCOL_ERROR));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(RST_STREAM, 1, _, 0x0, 1));
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR));
+ EXPECT_EQ(0, session.Send());
+
+ const std::string more_frames =
+ TestFrameSequence()
+ .Data(1, std::string(16 * 1024, 'x'), false)
+ .Data(1, std::string(16 * 1024, 'y'), false)
+ .Serialize();
+ // These bytes are counted against the connection flow control window but
+ // should be dropped right away and considered as consumed.
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, _)).Times(2);
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _)).Times(0);
+ EXPECT_CALL(visitor, OnDataForStream(1, _)).Times(0);
+
+ session.ProcessBytes(more_frames);
+ EXPECT_TRUE(session.want_write());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 0, _, 0x0, 0));
+ EXPECT_EQ(0, session.Send());
+ EXPECT_EQ(session.GetReceiveWindowSize(), kInitialFlowControlWindowSize);
+}
+
} // namespace test
} // namespace adapter
} // namespace http2
