gfe-relnote: Emit H3_SETTINGS_ERROR and H3_MISSING_SETTINGS when appropriate. Protected by gfe2_reloadable_flag_quic_enable_version_draft_25_v3 and gfe2_reloadable_flag_quic_enable_version_draft_27.
Emit H3_SETTINGS_ERROR when receiving a SETTINGS frame with duplicate setting
identifier. Emit H3_MISSING_SETTINGS when receiving a CANCEL_PUSH, GOAWAY,
MAX_PUSH_ID, PRIORITY_UPDATE, or unknown frame before a SETTINGS frame on the
control stream. Before this CL, H3_FRAME_UNEXPECTED was emitted for duplicate
SETTINGS or a PRIORITY_UPDATE or unknow frame before a SETTINGS, and no error
was signalled for a CANCEL_PUSH, GOAWAY, or MAX_PUSH_ID being the first frame on
the control stream.
If a DATA, HEADERS, or PUSH_PROMISE frame arrives on the control stream, keep
the error code H3_FRAME_UNEXPECTED, even if no SETTINGS frame has been received
yet. The specification has conflicting MUSTs, so either error code is
acceptable.
PiperOrigin-RevId: 304391962
Change-Id: If47b3940761e7955ee645bd59e4969302164a051
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc
index c1fa96c..87f9919 100644
--- a/quic/core/http/http_decoder.cc
+++ b/quic/core/http/http_decoder.cc
@@ -504,7 +504,8 @@
}
auto result = frame->values.insert({id, content});
if (!result.second) {
- RaiseError(QUIC_HTTP_FRAME_ERROR, "Duplicate setting identifier.");
+ RaiseError(QUIC_HTTP_DUPLICATE_SETTING_IDENTIFIER,
+ "Duplicate setting identifier.");
return false;
}
}
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc
index d97cbc1..2e8885c 100644
--- a/quic/core/http/http_decoder_test.cc
+++ b/quic/core/http/http_decoder_test.cc
@@ -448,7 +448,8 @@
EXPECT_EQ(input.size(), ProcessInput(input));
- EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_ERROR));
+ EXPECT_THAT(decoder_.error(),
+ IsError(QUIC_HTTP_DUPLICATE_SETTING_IDENTIFIER));
EXPECT_EQ("Duplicate setting identifier.", decoder_.error_detail());
}
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc
index 4e361f2..17c6ecc 100644
--- a/quic/core/http/quic_receive_control_stream.cc
+++ b/quic/core/http/quic_receive_control_stream.cc
@@ -63,6 +63,13 @@
spdy_session()->debug_visitor()->OnCancelPushFrameReceived(frame);
}
+ if (!settings_frame_received_) {
+ stream_delegate()->OnStreamError(
+ QUIC_HTTP_MISSING_SETTINGS_FRAME,
+ "CANCEL_PUSH frame received before SETTINGS.");
+ return false;
+ }
+
// TODO(b/151841240): Handle CANCEL_PUSH frames instead of ignoring them.
return true;
}
@@ -72,6 +79,13 @@
spdy_session()->debug_visitor()->OnMaxPushIdFrameReceived(frame);
}
+ if (!settings_frame_received_) {
+ stream_delegate()->OnStreamError(
+ QUIC_HTTP_MISSING_SETTINGS_FRAME,
+ "MAX_PUSH_ID frame received before SETTINGS.");
+ return false;
+ }
+
if (spdy_session()->perspective() == Perspective::IS_CLIENT) {
OnWrongFrame("Max Push Id");
return false;
@@ -84,11 +98,16 @@
}
bool QuicReceiveControlStream::OnGoAwayFrame(const GoAwayFrame& frame) {
- // TODO(bnc): Check if SETTINGS frame has been received.
if (spdy_session()->debug_visitor()) {
spdy_session()->debug_visitor()->OnGoAwayFrameReceived(frame);
}
+ if (!settings_frame_received_) {
+ stream_delegate()->OnStreamError(QUIC_HTTP_MISSING_SETTINGS_FRAME,
+ "GOAWAY frame received before SETTINGS.");
+ return false;
+ }
+
if (spdy_session()->perspective() == Perspective::IS_SERVER) {
OnWrongFrame("Go Away");
return false;
@@ -190,7 +209,7 @@
QuicByteCount /*header_length*/) {
if (!settings_frame_received_) {
stream_delegate()->OnStreamError(
- QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM,
+ QUIC_HTTP_MISSING_SETTINGS_FRAME,
"PRIORITY_UPDATE frame received before SETTINGS.");
return false;
}
@@ -250,9 +269,8 @@
}
if (!settings_frame_received_) {
- stream_delegate()->OnStreamError(
- QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM,
- "Unknown frame received before SETTINGS.");
+ stream_delegate()->OnStreamError(QUIC_HTTP_MISSING_SETTINGS_FRAME,
+ "Unknown frame received before SETTINGS.");
return false;
}
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc
index d9583ae..2914c92 100644
--- a/quic/core/http/quic_receive_control_stream_test.cc
+++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -256,7 +256,7 @@
EXPECT_CALL(
*connection_,
- CloseConnection(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM,
+ CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME,
"PRIORITY_UPDATE frame received before SETTINGS.", _))
.WillOnce(
Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
@@ -270,15 +270,25 @@
StrictMock<MockHttp3DebugVisitor> debug_visitor;
session_.set_debug_visitor(&debug_visitor);
- GoAwayFrame goaway;
- goaway.stream_id = 0x00;
+ QuicStreamOffset offset = 1;
+
+ // Receive SETTINGS frame.
+ SettingsFrame settings;
+ std::string settings_frame = EncodeSettings(settings);
+ EXPECT_CALL(debug_visitor, OnSettingsFrameReceived(settings));
+ receive_control_stream_->OnStreamFrame(
+ QuicStreamFrame(receive_control_stream_->id(), /* fin = */ false, offset,
+ settings_frame));
+ offset += settings_frame.length();
+
+ GoAwayFrame goaway{/* stream_id = */ 0};
std::unique_ptr<char[]> buffer;
QuicByteCount header_length =
HttpEncoder::SerializeGoAwayFrame(goaway, &buffer);
std::string data = std::string(buffer.get(), header_length);
- QuicStreamFrame frame(receive_control_stream_->id(), false, 1, data);
+ QuicStreamFrame frame(receive_control_stream_->id(), false, offset, data);
EXPECT_FALSE(session_.http3_goaway_received());
EXPECT_CALL(debug_visitor, OnGoAwayFrameReceived(goaway));
@@ -370,16 +380,34 @@
/* offset = */ 1 + settings_frame.size(), unknown_frame));
}
+TEST_P(QuicReceiveControlStreamTest, CancelPushFrameBeforeSettings) {
+ std::string cancel_push_frame = quiche::QuicheTextUtils::HexDecode(
+ "03" // type CANCEL_PUSH
+ "01" // payload length
+ "01"); // push ID
+
+ EXPECT_CALL(*connection_,
+ CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME,
+ "CANCEL_PUSH 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, cancel_push_frame));
+}
+
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.", _))
+ EXPECT_CALL(*connection_,
+ CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME,
+ "Unknown frame received before SETTINGS.", _))
.WillOnce(
Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _));
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc
index 164e6a1..2007806 100644
--- a/quic/core/http/quic_spdy_session_test.cc
+++ b/quic/core/http/quic_spdy_session_test.cc
@@ -2797,14 +2797,22 @@
EXPECT_EQ(receive_control_stream_id,
QuicSpdySessionPeer::GetReceiveControlStream(&session_)->id());
+ // First frame has to be SETTINGS.
+ std::string serialized_settings = EncodeSettings({});
+ QuicStreamFrame data2(receive_control_stream_id, /* fin = */ false, offset,
+ serialized_settings);
+ offset += serialized_settings.length();
+ EXPECT_CALL(debug_visitor, OnSettingsFrameReceived(_));
+ session_.OnStreamFrame(data2);
+
CancelPushFrame cancel_push{/* push_id = */ 0};
std::unique_ptr<char[]> buffer;
auto frame_length =
HttpEncoder::SerializeCancelPushFrame(cancel_push, &buffer);
- QuicStreamFrame data2(receive_control_stream_id, /* fin = */ false, offset,
+ QuicStreamFrame data3(receive_control_stream_id, /* fin = */ false, offset,
quiche::QuicheStringPiece(buffer.get(), frame_length));
EXPECT_CALL(debug_visitor, OnCancelPushFrameReceived(_));
- session_.OnStreamFrame(data2);
+ session_.OnStreamFrame(data3);
}
TEST_P(QuicSpdySessionTestServer, ServerPushEnabledDefaultValue) {
@@ -2973,6 +2981,9 @@
return;
}
+ StrictMock<MockHttp3DebugVisitor> debug_visitor;
+ session_.set_debug_visitor(&debug_visitor);
+
// Create control stream.
QuicStreamId receive_control_stream_id =
GetNthClientInitiatedUnidirectionalStreamId(transport_version(), 3);
@@ -2982,17 +2993,28 @@
QuicStreamFrame data1(receive_control_stream_id, /* fin = */ false, offset,
stream_type);
offset += stream_type.length();
+ EXPECT_CALL(debug_visitor,
+ OnPeerControlStreamCreated(receive_control_stream_id));
session_.OnStreamFrame(data1);
EXPECT_EQ(receive_control_stream_id,
QuicSpdySessionPeer::GetReceiveControlStream(&session_)->id());
+ // First frame has to be SETTINGS.
+ std::string serialized_settings = EncodeSettings({});
+ QuicStreamFrame data2(receive_control_stream_id, /* fin = */ false, offset,
+ serialized_settings);
+ offset += serialized_settings.length();
+ EXPECT_CALL(debug_visitor, OnSettingsFrameReceived(_));
+ session_.OnStreamFrame(data2);
+
CancelPushFrame cancel_push{/* push_id = */ 0};
std::unique_ptr<char[]> buffer;
auto frame_length =
HttpEncoder::SerializeCancelPushFrame(cancel_push, &buffer);
- QuicStreamFrame data2(receive_control_stream_id, /* fin = */ false, offset,
+ QuicStreamFrame data3(receive_control_stream_id, /* fin = */ false, offset,
quiche::QuicheStringPiece(buffer.get(), frame_length));
- session_.OnStreamFrame(data2);
+ EXPECT_CALL(debug_visitor, OnCancelPushFrameReceived(_));
+ session_.OnStreamFrame(data3);
}
TEST_P(QuicSpdySessionTestClient, SendInitialMaxPushIdIfSet) {
diff --git a/quic/core/quic_error_codes.cc b/quic/core/quic_error_codes.cc
index d897aac..db97d32 100644
--- a/quic/core/quic_error_codes.cc
+++ b/quic/core/quic_error_codes.cc
@@ -175,6 +175,8 @@
RETURN_STRING_LITERAL(QUIC_HTTP_SERVER_INITIATED_BIDIRECTIONAL_STREAM);
RETURN_STRING_LITERAL(QUIC_HTTP_STREAM_WRONG_DIRECTION);
RETURN_STRING_LITERAL(QUIC_HTTP_CLOSED_CRITICAL_STREAM);
+ RETURN_STRING_LITERAL(QUIC_HTTP_MISSING_SETTINGS_FRAME);
+ RETURN_STRING_LITERAL(QUIC_HTTP_DUPLICATE_SETTING_IDENTIFIER);
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 dc575d3..0548e9b 100644
--- a/quic/core/quic_error_codes.h
+++ b/quic/core/quic_error_codes.h
@@ -363,8 +363,7 @@
// 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.
+ // A second SETTINGS frame is received on the control stream.
QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM = 152,
// A second instance of a unidirectional stream of a certain type is created.
QUIC_HTTP_DUPLICATE_UNIDIRECTIONAL_STREAM = 153,
@@ -376,6 +375,10 @@
// Peer closes one of the six critical unidirectional streams (control, QPACK
// encoder or decoder, in either direction).
QUIC_HTTP_CLOSED_CRITICAL_STREAM = 156,
+ // The first frame received on the control stream is not a SETTINGS frame.
+ QUIC_HTTP_MISSING_SETTINGS_FRAME = 157,
+ // The received SETTINGS frame contains duplicate setting identifiers.
+ QUIC_HTTP_DUPLICATE_SETTING_IDENTIFIER = 158,
// HPACK header block decoding errors.
// Index varint beyond implementation limit.
@@ -412,7 +415,7 @@
QUIC_HPACK_COMPRESSED_HEADER_SIZE_EXCEEDS_LIMIT = 150,
// No error. Used as bound while iterating.
- QUIC_LAST_ERROR = 157,
+ QUIC_LAST_ERROR = 159,
};
// 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 5d69250..ce24d57 100644
--- a/quic/core/quic_types.cc
+++ b/quic/core/quic_types.cc
@@ -484,6 +484,14 @@
return {false,
{static_cast<uint64_t>(
QuicHttp3ErrorCode::IETF_QUIC_HTTP3_CLOSED_CRITICAL_STREAM)}};
+ case QUIC_HTTP_MISSING_SETTINGS_FRAME:
+ return {false,
+ {static_cast<uint64_t>(
+ QuicHttp3ErrorCode::IETF_QUIC_HTTP3_MISSING_SETTINGS)}};
+ case QUIC_HTTP_DUPLICATE_SETTING_IDENTIFIER:
+ return {false,
+ {static_cast<uint64_t>(
+ QuicHttp3ErrorCode::IETF_QUIC_HTTP3_SETTINGS_ERROR)}};
case QUIC_HPACK_INDEX_VARINT_ERROR:
return {false, {static_cast<uint64_t>(QUIC_HPACK_INDEX_VARINT_ERROR)}};
case QUIC_HPACK_NAME_LENGTH_VARINT_ERROR:
