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: