gfe-relnote: Enforce HTTP/3 frame ordering requirements on request and control streams. Protected by gfe2_reloadable_flag_quic_enable_version_draft_25_v3 and gfe2_reloadable_flag_quic_enable_version_draft_27.
Add new error codes and use them in request and control streams. Add new code
to control stream to enforce that first frame must be SETTINGS.
PiperOrigin-RevId: 300217136
Change-Id: Ib3ced012961f34c912ccd2061a3dc913f7bb96e6
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc
index 4f21498..d87ec83 100644
--- a/quic/core/http/quic_receive_control_stream.cc
+++ b/quic/core/http/quic_receive_control_stream.cc
@@ -122,8 +122,7 @@
bool OnUnknownFrameStart(uint64_t /* frame_type */,
QuicByteCount /* header_length */) override {
- // Ignore unknown frame types.
- return true;
+ return stream_->OnUnknownFrameStart();
}
bool OnUnknownFramePayload(quiche::QuicheStringPiece /* payload */) override {
@@ -190,9 +189,9 @@
bool QuicReceiveControlStream::OnSettingsFrameStart(
QuicByteCount /* header_length */) {
if (settings_frame_received_) {
- // TODO(renjietang): Change error code to HTTP_UNEXPECTED_FRAME.
- stream_delegate()->OnStreamError(QUIC_INVALID_STREAM_ID,
- "Settings frames are received twice.");
+ stream_delegate()->OnStreamError(
+ QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM,
+ "Settings frames are received twice.");
return false;
}
@@ -217,7 +216,7 @@
QuicByteCount /* header_length */) {
if (!settings_frame_received_) {
stream_delegate()->OnStreamError(
- QUIC_INVALID_STREAM_ID,
+ QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM,
"PRIORITY_UPDATE frame received before SETTINGS.");
return false;
}
@@ -263,4 +262,15 @@
return true;
}
+bool QuicReceiveControlStream::OnUnknownFrameStart() {
+ if (!settings_frame_received_) {
+ stream_delegate()->OnStreamError(
+ QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM,
+ "Unknown frame received before SETTINGS.");
+ return false;
+ }
+
+ return true;
+}
+
} // namespace quic
diff --git a/quic/core/http/quic_receive_control_stream.h b/quic/core/http/quic_receive_control_stream.h
index e77ccf9..d24bcd0 100644
--- a/quic/core/http/quic_receive_control_stream.h
+++ b/quic/core/http/quic_receive_control_stream.h
@@ -43,6 +43,7 @@
bool OnSettingsFrame(const SettingsFrame& settings);
bool OnPriorityUpdateFrameStart(QuicByteCount header_length);
bool OnPriorityUpdateFrame(const PriorityUpdateFrame& priority);
+ bool OnUnknownFrameStart();
// False until a SETTINGS frame is received.
bool settings_frame_received_;
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc
index 67a9a17..0997fc0 100644
--- a/quic/core/http/quic_receive_control_stream_test.cc
+++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -197,9 +197,10 @@
EXPECT_EQ(settings_frame.size() + 1, NumBytesConsumed());
// Second SETTINGS frame causes the connection to be closed.
- EXPECT_CALL(*connection_,
- CloseConnection(QUIC_INVALID_STREAM_ID,
- "Settings frames are received twice.", _))
+ EXPECT_CALL(
+ *connection_,
+ CloseConnection(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM,
+ "Settings frames are received twice.", _))
.WillOnce(
Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _));
@@ -255,7 +256,7 @@
EXPECT_CALL(
*connection_,
- CloseConnection(QUIC_INVALID_STREAM_ID,
+ CloseConnection(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM,
"PRIORITY_UPDATE frame received before SETTINGS.", _))
.WillOnce(
Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
@@ -310,18 +311,51 @@
// Regression test for b/137554973: unknown frames should be consumed.
TEST_P(QuicReceiveControlStreamTest, ConsumeUnknownFrame) {
+ EXPECT_EQ(1u, NumBytesConsumed());
+
+ // Receive SETTINGS frame.
+ SettingsFrame settings;
+ std::string settings_frame = EncodeSettings(settings);
+ receive_control_stream_->OnStreamFrame(
+ QuicStreamFrame(receive_control_stream_->id(), /* fin = */ false,
+ /* offset = */ 1, settings_frame));
+
+ // SETTINGS frame is consumed.
+ EXPECT_EQ(1 + settings_frame.size(), NumBytesConsumed());
+
+ // Receive unknown frame.
std::string unknown_frame = quiche::QuicheTextUtils::HexDecode(
"21" // reserved frame type
"03" // payload length
"666f6f"); // payload "foo"
- EXPECT_EQ(1u, NumBytesConsumed());
+ receive_control_stream_->OnStreamFrame(
+ QuicStreamFrame(receive_control_stream_->id(), /* fin = */ false,
+ /* offset = */ 1 + settings_frame.size(), unknown_frame));
+
+ // Unknown frame is consumed.
+ EXPECT_EQ(1 + settings_frame.size() + unknown_frame.size(),
+ NumBytesConsumed());
+}
+
+TEST_P(QuicReceiveControlStreamTest, UnknownFrameBeforeSettings) {
+ std::string unknown_frame = quiche::QuicheTextUtils::HexDecode(
+ "21" // reserved frame type
+ "03" // payload length
+ "666f6f"); // payload "foo"
+
+ EXPECT_CALL(
+ *connection_,
+ CloseConnection(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM,
+ "Unknown frame received before SETTINGS.", _))
+ .WillOnce(
+ Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
+ EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _));
+ EXPECT_CALL(session_, OnConnectionClosed(_, _));
receive_control_stream_->OnStreamFrame(
QuicStreamFrame(receive_control_stream_->id(), /* fin = */ false,
/* offset = */ 1, unknown_frame));
-
- EXPECT_EQ(unknown_frame.size() + 1, NumBytesConsumed());
}
} // namespace
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index b019b1e..9d2772a 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -844,9 +844,9 @@
bool QuicSpdyStream::OnDataFrameStart(QuicByteCount header_length) {
DCHECK(VersionUsesHttp3(transport_version()));
if (!headers_decompressed_ || trailers_decompressed_) {
- // TODO(b/124216424): Change error code to HTTP_UNEXPECTED_FRAME.
- stream_delegate()->OnStreamError(QUIC_INVALID_HEADERS_STREAM_DATA,
- "Unexpected DATA frame received.");
+ stream_delegate()->OnStreamError(
+ QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM,
+ "Unexpected DATA frame received.");
return false;
}
@@ -925,9 +925,8 @@
DCHECK(!qpack_decoded_headers_accumulator_);
if (trailers_decompressed_) {
- // TODO(b/124216424): Change error code to HTTP_UNEXPECTED_FRAME.
stream_delegate()->OnStreamError(
- QUIC_INVALID_HEADERS_STREAM_DATA,
+ QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM,
"HEADERS frame received after trailing HEADERS.");
return false;
}
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index 1d5305b..f1cb316 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -2473,10 +2473,9 @@
// Closing the connection is mocked out in tests. Instead, simply stop
// reading data at the stream level to prevent QuicSpdyStream from blowing up.
- // TODO(b/124216424): Change error code to HTTP_UNEXPECTED_FRAME.
EXPECT_CALL(
*connection_,
- CloseConnection(QUIC_INVALID_HEADERS_STREAM_DATA,
+ CloseConnection(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM,
"Unexpected DATA frame received.",
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET))
.WillOnce(InvokeWithoutArgs([this]() { stream_->StopReading(); }));
@@ -2523,10 +2522,9 @@
// Closing the connection is mocked out in tests. Instead, simply stop
// reading data at the stream level to prevent QuicSpdyStream from blowing up.
- // TODO(b/124216424): Change error code to HTTP_UNEXPECTED_FRAME.
EXPECT_CALL(
*connection_,
- CloseConnection(QUIC_INVALID_HEADERS_STREAM_DATA,
+ CloseConnection(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM,
"HEADERS frame received after trailing HEADERS.",
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET))
.WillOnce(InvokeWithoutArgs([this]() { stream_->StopReading(); }));
@@ -2574,10 +2572,9 @@
// Closing the connection is mocked out in tests. Instead, simply stop
// reading data at the stream level to prevent QuicSpdyStream from blowing up.
- // TODO(b/124216424): Change error code to HTTP_UNEXPECTED_FRAME.
EXPECT_CALL(
*connection_,
- CloseConnection(QUIC_INVALID_HEADERS_STREAM_DATA,
+ CloseConnection(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM,
"Unexpected DATA frame received.",
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET))
.WillOnce(InvokeWithoutArgs([this]() { stream_->StopReading(); }));
diff --git a/quic/core/quic_error_codes.cc b/quic/core/quic_error_codes.cc
index ce9866c..e16da69 100644
--- a/quic/core/quic_error_codes.cc
+++ b/quic/core/quic_error_codes.cc
@@ -169,6 +169,8 @@
RETURN_STRING_LITERAL(QUIC_HTTP_FRAME_ERROR);
RETURN_STRING_LITERAL(QUIC_HTTP_FRAME_UNEXPECTED_ON_SPDY_STREAM);
RETURN_STRING_LITERAL(QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM);
+ RETURN_STRING_LITERAL(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM);
+ RETURN_STRING_LITERAL(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM);
RETURN_STRING_LITERAL(QUIC_HPACK_INDEX_VARINT_ERROR);
RETURN_STRING_LITERAL(QUIC_HPACK_NAME_LENGTH_VARINT_ERROR);
RETURN_STRING_LITERAL(QUIC_HPACK_VALUE_LENGTH_VARINT_ERROR);
diff --git a/quic/core/quic_error_codes.h b/quic/core/quic_error_codes.h
index 7f250c3..557edfa 100644
--- a/quic/core/quic_error_codes.h
+++ b/quic/core/quic_error_codes.h
@@ -356,8 +356,16 @@
// Internal error codes for HTTP/3 errors.
QUIC_HTTP_FRAME_TOO_LARGE = 131,
QUIC_HTTP_FRAME_ERROR = 132,
+ // A frame that is never allowed on a request stream is received.
QUIC_HTTP_FRAME_UNEXPECTED_ON_SPDY_STREAM = 133,
+ // A frame that is never allowed on the control stream is received.
QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM = 134,
+ // An invalid sequence of frames normally allowed on a request stream is
+ // received.
+ QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM = 151,
+ // An invalid sequence of frames normally allowed on the control stream is
+ // received.
+ QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM = 152,
// HPACK header block decoding errors.
// Index varint beyond implementation limit.
@@ -394,7 +402,7 @@
QUIC_HPACK_COMPRESSED_HEADER_SIZE_EXCEEDS_LIMIT = 150,
// No error. Used as bound while iterating.
- QUIC_LAST_ERROR = 151,
+ QUIC_LAST_ERROR = 153,
};
// QuicErrorCodes is encoded as four octets on-the-wire when doing Google QUIC,
// or a varint62 when doing IETF QUIC. Ensure that its value does not exceed
diff --git a/quic/core/quic_types.cc b/quic/core/quic_types.cc
index ef5f5da..d1e76be 100644
--- a/quic/core/quic_types.cc
+++ b/quic/core/quic_types.cc
@@ -471,6 +471,14 @@
return {false,
{static_cast<uint64_t>(
QuicHttp3ErrorCode::IETF_QUIC_HTTP3_FRAME_UNEXPECTED)}};
+ case QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM:
+ return {false,
+ {static_cast<uint64_t>(
+ QuicHttp3ErrorCode::IETF_QUIC_HTTP3_FRAME_UNEXPECTED)}};
+ case QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM:
+ return {false,
+ {static_cast<uint64_t>(
+ QuicHttp3ErrorCode::IETF_QUIC_HTTP3_FRAME_UNEXPECTED)}};
case QUIC_HPACK_INDEX_VARINT_ERROR:
return {false, {static_cast<uint64_t>(QUIC_HPACK_INDEX_VARINT_ERROR)}};
case QUIC_HPACK_NAME_LENGTH_VARINT_ERROR:
