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: