Unify and refactor frame type logic in receiving control stream. An error is signalled under exactly the same conditions, but the error code and message might change. In particular, a MAX_PUSH_ID or ACCEPT_CH frame received by the wrong endpoint as the first frame resulted in a QUIC_HTTP_MISSING_SETTINGS_FRAME, while a DATA or HEADERS as the first frame resulted in a QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM before this CL. After this CL, any forbidden frame type (for the endpoint) results in QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM. Error messages are also changed to make the code simpler. OnUnrecoverableError() (used 2 times) is exactly the same as stream_delegate()->OnStreamError() (used 9 times), this CL changes both occurrences of the former into the latter for consistency. HttpDecoder::Visitor methods for frame processing other than *Start() cannot be called for disallowed frames, this CL adds QUICHE_NOTREACHED() to document that. The motivation is to prepare for changing the SETTINGS frame logic to accomodate a SETTINGS frame in ALPS, see https://vasilvv.github.io/httpbis-alps/draft-vvv-httpbis-alps.html#name-use-of-alps-in-http-3. PiperOrigin-RevId: 358846826 Change-Id: I0f361fa9899f1bd558603b804a6aad6f4f38b218
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc index 70e7f4a..35d5902 100644 --- a/quic/core/http/quic_receive_control_stream.cc +++ b/quic/core/http/quic_receive_control_stream.cc
@@ -62,7 +62,7 @@ } void QuicReceiveControlStream::OnError(HttpDecoder* decoder) { - OnUnrecoverableError(decoder->error(), decoder->error_detail()); + stream_delegate()->OnStreamError(decoder->error(), decoder->error_detail()); } bool QuicReceiveControlStream::OnCancelPushFrame(const CancelPushFrame& frame) { @@ -70,15 +70,8 @@ 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; + return ValidateFrameType(HttpFrameType::CANCEL_PUSH); } bool QuicReceiveControlStream::OnMaxPushIdFrame(const MaxPushIdFrame& frame) { @@ -86,15 +79,7 @@ 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"); + if (!ValidateFrameType(HttpFrameType::MAX_PUSH_ID)) { return false; } @@ -106,9 +91,7 @@ spdy_session()->debug_visitor()->OnGoAwayFrameReceived(frame); } - if (!settings_frame_received_) { - stream_delegate()->OnStreamError(QUIC_HTTP_MISSING_SETTINGS_FRAME, - "GOAWAY frame received before SETTINGS."); + if (!ValidateFrameType(HttpFrameType::GOAWAY)) { return false; } @@ -118,16 +101,7 @@ bool QuicReceiveControlStream::OnSettingsFrameStart( QuicByteCount /*header_length*/) { - if (settings_frame_received_) { - stream_delegate()->OnStreamError( - QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM, - "Settings frames are received twice."); - return false; - } - - settings_frame_received_ = true; - - return true; + return ValidateFrameType(HttpFrameType::SETTINGS); } bool QuicReceiveControlStream::OnSettingsFrame(const SettingsFrame& frame) { @@ -139,18 +113,17 @@ bool QuicReceiveControlStream::OnDataFrameStart(QuicByteCount /*header_length*/, QuicByteCount /*payload_length*/) { - OnWrongFrame("Data"); - return false; + return ValidateFrameType(HttpFrameType::DATA); } bool QuicReceiveControlStream::OnDataFramePayload( absl::string_view /*payload*/) { - OnWrongFrame("Data"); + QUICHE_NOTREACHED(); return false; } bool QuicReceiveControlStream::OnDataFrameEnd() { - OnWrongFrame("Data"); + QUICHE_NOTREACHED(); return false; } @@ -158,55 +131,47 @@ QuicByteCount /*header_length*/, QuicByteCount /*payload_length*/) { - OnWrongFrame("Headers"); - return false; + return ValidateFrameType(HttpFrameType::HEADERS); } bool QuicReceiveControlStream::OnHeadersFramePayload( absl::string_view /*payload*/) { - OnWrongFrame("Headers"); + QUICHE_NOTREACHED(); return false; } bool QuicReceiveControlStream::OnHeadersFrameEnd() { - OnWrongFrame("Headers"); + QUICHE_NOTREACHED(); return false; } bool QuicReceiveControlStream::OnPushPromiseFrameStart( QuicByteCount /*header_length*/) { - OnWrongFrame("Push Promise"); - return false; + return ValidateFrameType(HttpFrameType::PUSH_PROMISE); } bool QuicReceiveControlStream::OnPushPromiseFramePushId( PushId /*push_id*/, QuicByteCount /*push_id_length*/, QuicByteCount /*header_block_length*/) { - OnWrongFrame("Push Promise"); + QUICHE_NOTREACHED(); return false; } bool QuicReceiveControlStream::OnPushPromiseFramePayload( absl::string_view /*payload*/) { - OnWrongFrame("Push Promise"); + QUICHE_NOTREACHED(); return false; } bool QuicReceiveControlStream::OnPushPromiseFrameEnd() { - OnWrongFrame("Push Promise"); + QUICHE_NOTREACHED(); return false; } bool QuicReceiveControlStream::OnPriorityUpdateFrameStart( QuicByteCount /*header_length*/) { - if (!settings_frame_received_) { - stream_delegate()->OnStreamError( - QUIC_HTTP_MISSING_SETTINGS_FRAME, - "PRIORITY_UPDATE frame received before SETTINGS."); - return false; - } - return true; + return ValidateFrameType(HttpFrameType::PRIORITY_UPDATE); } bool QuicReceiveControlStream::OnPriorityUpdateFrame( @@ -254,19 +219,7 @@ bool QuicReceiveControlStream::OnAcceptChFrameStart( QuicByteCount /* header_length */) { - if (!settings_frame_received_) { - stream_delegate()->OnStreamError( - QUIC_HTTP_MISSING_SETTINGS_FRAME, - "ACCEPT_CH frame received before SETTINGS."); - return false; - } - - if (spdy_session()->perspective() == Perspective::IS_SERVER) { - OnWrongFrame("ACCEPT_CH"); - return false; - } - - return true; + return ValidateFrameType(HttpFrameType::ACCEPT_CH); } bool QuicReceiveControlStream::OnAcceptChFrame(const AcceptChFrame& frame) { @@ -289,13 +242,7 @@ payload_length); } - if (!settings_frame_received_) { - stream_delegate()->OnStreamError(QUIC_HTTP_MISSING_SETTINGS_FRAME, - "Unknown frame received before SETTINGS."); - return false; - } - - return true; + return ValidateFrameType(static_cast<HttpFrameType>(frame_type)); } bool QuicReceiveControlStream::OnUnknownFramePayload( @@ -309,10 +256,43 @@ return true; } -void QuicReceiveControlStream::OnWrongFrame(absl::string_view frame_type) { - OnUnrecoverableError( - QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, - absl::StrCat(frame_type, " frame received on control stream")); +bool QuicReceiveControlStream::ValidateFrameType(HttpFrameType frame_type) { + // Certain frame types are forbidden. + if ((frame_type == HttpFrameType::DATA || + frame_type == HttpFrameType::HEADERS || + frame_type == HttpFrameType::PUSH_PROMISE) || + (spdy_session()->perspective() == Perspective::IS_CLIENT && + frame_type == HttpFrameType::MAX_PUSH_ID) || + (GetQuicReloadableFlag(quic_parse_accept_ch_frame) && + spdy_session()->perspective() == Perspective::IS_SERVER && + frame_type == HttpFrameType::ACCEPT_CH)) { + stream_delegate()->OnStreamError( + QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, + absl::StrCat("Invalid frame type ", static_cast<int>(frame_type), + " received on control stream.")); + return false; + } + + if (settings_frame_received_) { + if (frame_type == HttpFrameType::SETTINGS) { + // SETTINGS frame may only be the first frame on the control stream. + stream_delegate()->OnStreamError( + QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM, + "SETTINGS frame can only be received once."); + return false; + } + return true; + } + + if (frame_type == HttpFrameType::SETTINGS) { + settings_frame_received_ = true; + return true; + } + stream_delegate()->OnStreamError( + QUIC_HTTP_MISSING_SETTINGS_FRAME, + absl::StrCat("First frame received on control stream is type ", + static_cast<int>(frame_type), ", but it must be SETTINGS.")); + return false; } } // namespace quic
diff --git a/quic/core/http/quic_receive_control_stream.h b/quic/core/http/quic_receive_control_stream.h index 8699021..326dba3 100644 --- a/quic/core/http/quic_receive_control_stream.h +++ b/quic/core/http/quic_receive_control_stream.h
@@ -69,7 +69,10 @@ QuicSpdySession* spdy_session() { return spdy_session_; } private: - void OnWrongFrame(absl::string_view frame_type); + // Called when a frame of allowed type is received. Returns true if the frame + // is allowed in this position. Returns false and resets the stream + // otherwise. + bool ValidateFrameType(HttpFrameType frame_type); // 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 fdd74b0..3e4532f 100644 --- a/quic/core/http/quic_receive_control_stream_test.cc +++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -206,7 +206,7 @@ EXPECT_CALL( *connection_, CloseConnection(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_CONTROL_STREAM, - "Settings frames are received twice.", _)) + "SETTINGS frame can only be received once.", _)) .WillOnce( Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _)); @@ -258,10 +258,11 @@ QuicStreamFrame data(receive_control_stream_->id(), /* fin = */ false, /* offset = */ 1, serialized_frame); - EXPECT_CALL( - *connection_, - CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME, - "PRIORITY_UPDATE frame received before SETTINGS.", _)) + EXPECT_CALL(*connection_, + CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME, + "First frame received on control stream is type " + "15, but it must be SETTINGS.", + _)) .WillOnce( Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _)); @@ -385,7 +386,9 @@ EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME, - "CANCEL_PUSH frame received before SETTINGS.", _)) + "First frame received on control stream is type " + "3, but it must be SETTINGS.", + _)) .WillOnce( Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _)); @@ -401,14 +404,23 @@ "4089" // type (ACCEPT_CH) "00"); // length - EXPECT_CALL(*connection_, - CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME, - GetQuicReloadableFlag(quic_parse_accept_ch_frame) - ? "ACCEPT_CH frame received before SETTINGS." - : "Unknown frame received before SETTINGS.", - _)) - .WillOnce( - Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); + if (GetQuicReloadableFlag(quic_parse_accept_ch_frame) && + perspective() == Perspective::IS_SERVER) { + EXPECT_CALL(*connection_, + CloseConnection( + QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, + "Invalid frame type 137 received on control stream.", _)) + .WillOnce( + Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); + } else { + EXPECT_CALL(*connection_, + CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME, + "First frame received on control stream is " + "type 137, but it must be SETTINGS.", + _)) + .WillOnce( + Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); + } EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _)); EXPECT_CALL(session_, OnConnectionClosed(_, _)); @@ -441,10 +453,10 @@ if (perspective() == Perspective::IS_CLIENT) { EXPECT_CALL(debug_visitor, OnAcceptChFrameReceived(_)); } else { - EXPECT_CALL( - *connection_, - CloseConnection(QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, - "ACCEPT_CH frame received on control stream", _)) + EXPECT_CALL(*connection_, + CloseConnection( + QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, + "Invalid frame type 137 received on control stream.", _)) .WillOnce( Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _)); @@ -468,7 +480,9 @@ EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME, - "Unknown frame received before SETTINGS.", _)) + "First frame received on control stream is type " + "33, but it must be SETTINGS.", + _)) .WillOnce( Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _));