Validate that received GOAWAY last-stream-ID fields are non-increasing in oghttp2.
This CL is a follow-up to a comment in cl/439588951 about enforcing this
section of the specification:
"An endpoint MAY send multiple GOAWAY frames if circumstances change. [...]
Endpoints MUST NOT increase the value they send in the last stream identifier,
since the peers might already have retried unprocessed requests on another
connection." (From https://httpwg.org/specs/rfc7540.html#GOAWAY)
This CL updates OgHttp2Session to follow this part of the specification, which
also increases parity with nghttp2 behavior:
http://google3/third_party/nghttp2/src/lib/nghttp2_session.c;l=4730-4738;rcl=439727518
PiperOrigin-RevId: 441023801
diff --git a/quiche/http2/adapter/http2_util.cc b/quiche/http2/adapter/http2_util.cc
index 7cca26b..963d264 100644
--- a/quiche/http2/adapter/http2_util.cc
+++ b/quiche/http2/adapter/http2_util.cc
@@ -99,6 +99,8 @@
return "ExceededMaxConcurrentStreams";
case ConnectionError::kFlowControlError:
return "FlowControlError";
+ case ConnectionError::kInvalidGoAwayLastStreamId:
+ return "InvalidGoAwayLastStreamId";
}
return "UnknownConnectionError";
}
diff --git a/quiche/http2/adapter/http2_visitor_interface.h b/quiche/http2/adapter/http2_visitor_interface.h
index 012d91a..2ec68c5 100644
--- a/quiche/http2/adapter/http2_visitor_interface.h
+++ b/quiche/http2/adapter/http2_visitor_interface.h
@@ -80,6 +80,8 @@
kExceededMaxConcurrentStreams,
// The peer caused a flow control error.
kFlowControlError,
+ // The peer sent a GOAWAY with an invalid last-stream-ID field.
+ kInvalidGoAwayLastStreamId,
};
virtual void OnConnectionError(ConnectionError error) = 0;
diff --git a/quiche/http2/adapter/nghttp2_adapter_test.cc b/quiche/http2/adapter/nghttp2_adapter_test.cc
index 4c1c4ec..f542429 100644
--- a/quiche/http2/adapter/nghttp2_adapter_test.cc
+++ b/quiche/http2/adapter/nghttp2_adapter_test.cc
@@ -1702,6 +1702,67 @@
EXPECT_THAT(visitor.data(), testing::IsEmpty());
}
+TEST(NgHttp2AdapterTest, ClientReceivesMultipleGoAwaysWithIncreasingStreamId) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
+
+ testing::InSequence s;
+
+ const std::vector<Header> headers1 =
+ ToHeaders({{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}});
+
+ const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr);
+ ASSERT_GT(stream_id1, 0);
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0));
+
+ int result = adapter->Send();
+ EXPECT_EQ(0, result);
+ absl::string_view data = visitor.data();
+ EXPECT_THAT(data, testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix));
+ data.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix));
+ EXPECT_THAT(data, EqualsFrames({SpdyFrameType::HEADERS}));
+ visitor.Clear();
+
+ const std::string frames =
+ TestFrameSequence()
+ .ServerPreface()
+ .GoAway(0, Http2ErrorCode::HTTP2_NO_ERROR, "")
+ .GoAway(0, Http2ErrorCode::ENHANCE_YOUR_CALM, "")
+ .GoAway(1, Http2ErrorCode::INTERNAL_ERROR, "")
+ .Serialize();
+
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+ EXPECT_CALL(visitor, OnFrameHeader(0, _, GOAWAY, 0));
+ EXPECT_CALL(visitor, OnGoAway(0, Http2ErrorCode::HTTP2_NO_ERROR, ""));
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::REFUSED_STREAM));
+ EXPECT_CALL(visitor, OnFrameHeader(0, _, GOAWAY, 0));
+ EXPECT_CALL(visitor, OnGoAway(0, Http2ErrorCode::ENHANCE_YOUR_CALM, ""));
+ EXPECT_CALL(visitor, OnFrameHeader(0, _, GOAWAY, 0));
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(0, Http2VisitorInterface::InvalidFrameError::kProtocol));
+
+ const int64_t frames_result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(frames.size(), static_cast<size_t>(frames_result));
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(GOAWAY, 0, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+
+ EXPECT_TRUE(adapter->want_write());
+ result = adapter->Send();
+ EXPECT_EQ(0, result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::GOAWAY}));
+}
+
TEST(NgHttp2AdapterTest, ClientReceivesGoAwayWithPendingStreams) {
DataSavingVisitor visitor;
auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
diff --git a/quiche/http2/adapter/oghttp2_adapter_test.cc b/quiche/http2/adapter/oghttp2_adapter_test.cc
index c2ae248..1c178b5 100644
--- a/quiche/http2/adapter/oghttp2_adapter_test.cc
+++ b/quiche/http2/adapter/oghttp2_adapter_test.cc
@@ -2075,6 +2075,74 @@
EXPECT_THAT(visitor.data(), testing::IsEmpty());
}
+TEST(OgHttp2AdapterTest, ClientReceivesMultipleGoAwaysWithIncreasingStreamId) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options{.perspective = Perspective::kClient};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+
+ testing::InSequence s;
+
+ const std::vector<Header> headers1 =
+ ToHeaders({{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}});
+
+ const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr);
+ ASSERT_GT(stream_id1, 0);
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0));
+
+ int result = adapter->Send();
+ EXPECT_EQ(0, result);
+ absl::string_view data = visitor.data();
+ EXPECT_THAT(data, testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix));
+ data.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix));
+ EXPECT_THAT(data,
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::HEADERS}));
+ visitor.Clear();
+
+ const std::string frames =
+ TestFrameSequence()
+ .ServerPreface()
+ .GoAway(0, Http2ErrorCode::HTTP2_NO_ERROR, "")
+ .GoAway(0, Http2ErrorCode::ENHANCE_YOUR_CALM, "")
+ .GoAway(1, Http2ErrorCode::INTERNAL_ERROR, "")
+ .Serialize();
+
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+ EXPECT_CALL(visitor, OnFrameHeader(0, _, GOAWAY, 0));
+ EXPECT_CALL(visitor, OnGoAway(0, Http2ErrorCode::HTTP2_NO_ERROR, ""));
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::REFUSED_STREAM));
+ EXPECT_CALL(visitor, OnFrameHeader(0, _, GOAWAY, 0));
+ EXPECT_CALL(visitor, OnGoAway(0, Http2ErrorCode::ENHANCE_YOUR_CALM, ""));
+ EXPECT_CALL(visitor, OnFrameHeader(0, _, GOAWAY, 0));
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(0, Http2VisitorInterface::InvalidFrameError::kProtocol));
+ // The oghttp2 stack also signals the error via OnConnectionError().
+ EXPECT_CALL(visitor,
+ OnConnectionError(ConnectionError::kInvalidGoAwayLastStreamId));
+
+ const int64_t frames_result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(frames.size(), static_cast<size_t>(frames_result));
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(GOAWAY, 0, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+
+ EXPECT_TRUE(adapter->want_write());
+ result = adapter->Send();
+ EXPECT_EQ(0, result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::GOAWAY}));
+}
+
TEST(OgHttp2AdapterTest, ClientReceivesGoAwayWithPendingStreams) {
DataSavingVisitor visitor;
OgHttp2Adapter::Options options{.perspective = Perspective::kClient};
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc
index 1c84c3e..7d99ab2 100644
--- a/quiche/http2/adapter/oghttp2_session.cc
+++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -1301,8 +1301,23 @@
void OgHttp2Session::OnGoAway(spdy::SpdyStreamId last_accepted_stream_id,
spdy::SpdyErrorCode error_code) {
+ if (received_goaway_ &&
+ last_accepted_stream_id >
+ static_cast<spdy::SpdyStreamId>(received_goaway_stream_id_)) {
+ // This GOAWAY has a higher `last_accepted_stream_id` than a previous
+ // GOAWAY, a connection-level spec violation.
+ const bool ok = visitor_.OnInvalidFrame(
+ kConnectionStreamId,
+ Http2VisitorInterface::InvalidFrameError::kProtocol);
+ if (!ok) {
+ fatal_visitor_callback_failure_ = true;
+ }
+ LatchErrorAndNotify(Http2ErrorCode::PROTOCOL_ERROR,
+ ConnectionError::kInvalidGoAwayLastStreamId);
+ return;
+ }
+
received_goaway_ = true;
- // TODO(diannahu): Validate that `last_accepted_stream_id` is non-increasing.
received_goaway_stream_id_ = last_accepted_stream_id;
const bool result = visitor_.OnGoAway(last_accepted_stream_id,
TranslateErrorCode(error_code), "");
