Treat wrong frame sequences as connection errors.
This CL introduces a new ConnectionError value, kWrongFrameSequence, and makes
OgHttp2Session invoke visitor_.OnConnectionError(kWrongFrameSequence) when
receiving a WINDOW_UPDATE, DATA, or RST_STREAM frame for an idle stream. An
idle stream is defined as one with a stream ID above the stream ID watermark
introduced in cl/401338105.
PiperOrigin-RevId: 403181489
diff --git a/http2/adapter/http2_util.cc b/http2/adapter/http2_util.cc
index e450490..63d9ea3 100644
--- a/http2/adapter/http2_util.cc
+++ b/http2/adapter/http2_util.cc
@@ -86,6 +86,8 @@
return "HeaderError";
case ConnectionError::kInvalidNewStreamId:
return "InvalidNewStreamId";
+ case ConnectionError::kWrongFrameSequence:
+ return "kWrongFrameSequence";
}
}
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h
index 4900a6a..4b07704 100644
--- a/http2/adapter/http2_visitor_interface.h
+++ b/http2/adapter/http2_visitor_interface.h
@@ -72,6 +72,8 @@
kHeaderError,
// The peer attempted to open a stream with an invalid stream ID.
kInvalidNewStreamId,
+ // The peer sent a frame that is invalid on an idle stream (before HEADERS).
+ kWrongFrameSequence,
};
virtual void OnConnectionError(ConnectionError error) = 0;
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index f698d8d..1748af8 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -2429,6 +2429,124 @@
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS}));
}
+TEST(NgHttp2AdapterTest, ServerForbidsWindowUpdateOnIdleStream) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+
+ EXPECT_EQ(0, adapter->GetHighestReceivedStreamId());
+
+ const std::string frames =
+ TestFrameSequence().ClientPreface().WindowUpdate(1, 42).Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, 4, WINDOW_UPDATE, 0));
+ EXPECT_CALL(visitor, OnInvalidFrame(1, _));
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(frames.size(), result);
+
+ EXPECT_EQ(0, adapter->GetHighestReceivedStreamId());
+
+ EXPECT_TRUE(adapter->want_write());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(GOAWAY, 0, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+
+ int send_result = adapter->Send();
+ // Some bytes should have been serialized.
+ EXPECT_EQ(0, send_result);
+ // The GOAWAY apparently causes the SETTINGS ack to be dropped.
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::GOAWAY}));
+}
+
+TEST(NgHttp2AdapterTest, ServerForbidsDataOnIdleStream) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+
+ EXPECT_EQ(0, adapter->GetHighestReceivedStreamId());
+
+ const std::string frames = TestFrameSequence()
+ .ClientPreface()
+ .Data(1, "Sorry, out of order")
+ .Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(frames.size(), result);
+
+ EXPECT_EQ(0, adapter->GetHighestReceivedStreamId());
+
+ EXPECT_TRUE(adapter->want_write());
+
+ // In this case, nghttp2 goes straight to GOAWAY and does not invoke the
+ // invalid frame callback.
+ EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(GOAWAY, 0, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+
+ int send_result = adapter->Send();
+ // Some bytes should have been serialized.
+ EXPECT_EQ(0, send_result);
+ // The GOAWAY apparently causes the SETTINGS ack to be dropped.
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::GOAWAY}));
+}
+
+TEST(NgHttp2AdapterTest, ServerForbidsRstStreamOnIdleStream) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+
+ EXPECT_EQ(0, adapter->GetHighestReceivedStreamId());
+
+ const std::string frames =
+ TestFrameSequence()
+ .ClientPreface()
+ .RstStream(1, Http2ErrorCode::ENHANCE_YOUR_CALM)
+ .Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, RST_STREAM, 0));
+ EXPECT_CALL(visitor, OnInvalidFrame(1, _));
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(frames.size(), result);
+
+ EXPECT_EQ(0, adapter->GetHighestReceivedStreamId());
+
+ EXPECT_TRUE(adapter->want_write());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(GOAWAY, 0, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+
+ int send_result = adapter->Send();
+ // Some bytes should have been serialized.
+ EXPECT_EQ(0, send_result);
+ // The GOAWAY apparently causes the SETTINGS ack to be dropped.
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::GOAWAY}));
+}
+
} // namespace
} // namespace test
} // namespace adapter
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index 34a39eb..f62f9e2 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -1766,6 +1766,146 @@
spdy::SpdyFrameType::GOAWAY}));
}
+TEST(OgHttp2AdapterServerTest, ServerForbidsWindowUpdateOnIdleStream) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+
+ EXPECT_EQ(0, adapter->GetHighestReceivedStreamId());
+
+ const std::string frames =
+ TestFrameSequence().ClientPreface().WindowUpdate(1, 42).Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, 4, WINDOW_UPDATE, 0));
+ EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kWrongFrameSequence));
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_LT(result, 0);
+
+ EXPECT_EQ(1, adapter->GetHighestReceivedStreamId());
+
+ EXPECT_TRUE(adapter->want_write());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(GOAWAY, 0, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+
+ int send_result = adapter->Send();
+ // Some bytes should have been serialized.
+ EXPECT_EQ(0, send_result);
+ // SETTINGS, SETTINGS ack, and GOAWAY.
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::GOAWAY}));
+}
+
+TEST(OgHttp2AdapterServerTest, ServerForbidsDataOnIdleStream) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+
+ EXPECT_EQ(0, adapter->GetHighestReceivedStreamId());
+
+ const std::string frames = TestFrameSequence()
+ .ClientPreface()
+ .Data(1, "Sorry, out of order")
+ .Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0));
+ EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kWrongFrameSequence));
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_LT(result, 0);
+
+ EXPECT_EQ(1, adapter->GetHighestReceivedStreamId());
+
+ EXPECT_TRUE(adapter->want_write());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(GOAWAY, 0, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+
+ int send_result = adapter->Send();
+ // Some bytes should have been serialized.
+ EXPECT_EQ(0, send_result);
+ // SETTINGS, SETTINGS ack, and GOAWAY.
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::GOAWAY}));
+}
+
+TEST(OgHttp2AdapterServerTest, ServerForbidsRstStreamOnIdleStream) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+
+ EXPECT_EQ(0, adapter->GetHighestReceivedStreamId());
+
+ const std::string frames =
+ TestFrameSequence()
+ .ClientPreface()
+ .RstStream(1, Http2ErrorCode::ENHANCE_YOUR_CALM)
+ .Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, RST_STREAM, 0));
+ EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kWrongFrameSequence));
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_LT(result, 0);
+
+ EXPECT_EQ(1, adapter->GetHighestReceivedStreamId());
+
+ EXPECT_TRUE(adapter->want_write());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(GOAWAY, 0, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+
+ int send_result = adapter->Send();
+ // Some bytes should have been serialized.
+ EXPECT_EQ(0, send_result);
+ // SETTINGS, SETTINGS ack, and GOAWAY.
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::GOAWAY}));
+}
+
} // namespace
} // namespace test
} // namespace adapter
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index 8c5e6c8..181fd91 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -721,6 +721,12 @@
void OgHttp2Session::OnDataFrameHeader(spdy::SpdyStreamId stream_id,
size_t length, bool /*fin*/) {
+ if (static_cast<Http2StreamId>(stream_id) > highest_processed_stream_id_) {
+ // Receiving DATA before HEADERS is a connection error.
+ LatchErrorAndNotify(Http2ErrorCode::PROTOCOL_ERROR,
+ ConnectionError::kWrongFrameSequence);
+ return;
+ }
const bool result = visitor_.OnBeginDataForStream(stream_id, length);
if (!result) {
decoder_.StopProcessing();
@@ -730,7 +736,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.
MarkDataBuffered(stream_id, len);
+
+ if (static_cast<Http2StreamId>(stream_id) > highest_processed_stream_id_) {
+ // Receiving DATA before HEADERS is a connection error; the visitor was
+ // informed in OnDataFrameHeader().
+ return;
+ }
+
const bool result =
visitor_.OnDataForStream(stream_id, absl::string_view(data, len));
if (!result) {
@@ -798,6 +812,12 @@
iter->second.half_closed_remote = true;
iter->second.outbound_body = nullptr;
write_scheduler_.UnregisterStream(stream_id);
+ } else if (static_cast<Http2StreamId>(stream_id) >
+ highest_processed_stream_id_) {
+ // Receiving RST_STREAM before HEADERS is a connection error.
+ LatchErrorAndNotify(Http2ErrorCode::PROTOCOL_ERROR,
+ ConnectionError::kWrongFrameSequence);
+ return;
}
visitor_.OnRstStream(stream_id, TranslateErrorCode(error_code));
CloseStream(stream_id, TranslateErrorCode(error_code));
@@ -873,6 +893,13 @@
auto it = stream_map_.find(stream_id);
if (it == stream_map_.end()) {
QUICHE_VLOG(1) << "Stream " << stream_id << " not found!";
+ if (static_cast<Http2StreamId>(stream_id) >
+ highest_processed_stream_id_) {
+ // Receiving WINDOW_UPDATE before HEADERS is a connection error.
+ LatchErrorAndNotify(Http2ErrorCode::PROTOCOL_ERROR,
+ ConnectionError::kWrongFrameSequence);
+ return;
+ }
} else {
if (it->second.send_window == 0) {
// The stream was blocked on flow control.
diff --git a/http2/adapter/oghttp2_session_test.cc b/http2/adapter/oghttp2_session_test.cc
index 4fdee88..8951273 100644
--- a/http2/adapter/oghttp2_session_test.cc
+++ b/http2/adapter/oghttp2_session_test.cc
@@ -72,8 +72,6 @@
EXPECT_EQ(0, session.GetHpackDecoderDynamicTableSize());
- // Should OgHttp2Session require that streams 1 and 3 have been created?
-
// Submit a request to ensure the first stream is created.
const char* kSentinel1 = "arbitrary pointer 1";
auto body1 = absl::make_unique<TestDataFrameSource>(visitor, true);
@@ -87,6 +85,15 @@
std::move(body1), const_cast<char*>(kSentinel1));
EXPECT_EQ(stream_id, 1);
+ // Submit another request to ensure the next stream is created.
+ int stream_id2 =
+ session.SubmitRequest(ToHeaders({{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/two"}}),
+ nullptr, nullptr);
+ EXPECT_EQ(stream_id2, 3);
+
const std::string stream_frames =
TestFrameSequence()
.Headers(stream_id,
@@ -95,7 +102,7 @@
{"date", "Tue, 6 Apr 2021 12:54:01 GMT"}},
/*fin=*/false)
.Data(stream_id, "This is the response body.")
- .RstStream(3, Http2ErrorCode::INTERNAL_ERROR)
+ .RstStream(stream_id2, Http2ErrorCode::INTERNAL_ERROR)
.GoAway(5, Http2ErrorCode::ENHANCE_YOUR_CALM, "calm down!!")
.Serialize();
@@ -111,14 +118,15 @@
EXPECT_CALL(visitor, OnBeginDataForStream(stream_id, 26));
EXPECT_CALL(visitor,
OnDataForStream(stream_id, "This is the response body."));
- EXPECT_CALL(visitor, OnFrameHeader(3, 4, RST_STREAM, 0));
- EXPECT_CALL(visitor, OnRstStream(3, Http2ErrorCode::INTERNAL_ERROR));
- EXPECT_CALL(visitor, OnCloseStream(3, Http2ErrorCode::INTERNAL_ERROR));
+ EXPECT_CALL(visitor, OnFrameHeader(stream_id2, 4, RST_STREAM, 0));
+ EXPECT_CALL(visitor, OnRstStream(stream_id2, Http2ErrorCode::INTERNAL_ERROR));
+ EXPECT_CALL(visitor,
+ OnCloseStream(stream_id2, Http2ErrorCode::INTERNAL_ERROR));
EXPECT_CALL(visitor, OnFrameHeader(0, 19, GOAWAY, 0));
EXPECT_CALL(visitor, OnGoAway(5, Http2ErrorCode::ENHANCE_YOUR_CALM, ""));
const int64_t stream_result = session.ProcessBytes(stream_frames);
EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result));
- EXPECT_EQ(3, session.GetHighestReceivedStreamId());
+ EXPECT_EQ(stream_id2, session.GetHighestReceivedStreamId());
// The first stream is active and has received some data.
EXPECT_GT(kInitialFlowControlWindowSize,
