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(_, _, _));
