gfe-relnote: Close connection if incoming MAX_PUSH_ID frame tries to reduce maximum push ID. Behavior change in IETF QUIC only, protected by gfe2_reloadable_flag_quic_enable_version_draft_25_v3 and gfe2_reloadable_flag_quic_enable_version_draft_27. As Renjie suggested at cl/302687788, this is done in QuicSpdySession instead of QuicReceiveControlStream. PiperOrigin-RevId: 306424146 Change-Id: I51ee39b6ac9c8ead8616009d60f1dd5c933c25ab
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc index 7b54756..d24e3dd 100644 --- a/quic/core/http/quic_receive_control_stream.cc +++ b/quic/core/http/quic_receive_control_stream.cc
@@ -91,10 +91,7 @@ return false; } - // TODO(b/124216424): Signal error if received push ID is smaller than a - // previously received value. - spdy_session()->OnMaxPushIdFrame(frame.push_id); - return true; + return spdy_session()->OnMaxPushIdFrame(frame.push_id); } bool QuicReceiveControlStream::OnGoAwayFrame(const GoAwayFrame& frame) {
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc index c7bb953..f6dd0a7 100644 --- a/quic/core/http/quic_spdy_session.cc +++ b/quic/core/http/quic_spdy_session.cc
@@ -1255,14 +1255,23 @@ quiche::QuicheOptional<PushId> old_max_push_id = max_push_id_; max_push_id_ = max_push_id; - if (!old_max_push_id.has_value() || - max_push_id_.value() > old_max_push_id.value()) { + if (!old_max_push_id.has_value() || max_push_id > old_max_push_id.value()) { OnCanCreateNewOutgoingStream(true); return true; } // Equal value is not considered an error. - return max_push_id_.value() >= old_max_push_id.value(); + if (max_push_id < old_max_push_id.value()) { + CloseConnectionWithDetails( + QUIC_HTTP_INVALID_MAX_PUSH_ID, + quiche::QuicheStrCat( + "MAX_PUSH_ID received with value ", max_push_id, + " which is smaller that previously received value ", + old_max_push_id.value())); + return false; + } + + return true; } void QuicSpdySession::SendMaxPushId() {
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index 307dabb..63fb18d 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -424,6 +424,15 @@ return std::string(priority_buffer.get(), priority_frame_length); } + std::string SerializeMaxPushIdFrame(PushId push_id) { + MaxPushIdFrame max_push_id_frame; + max_push_id_frame.push_id = push_id; + std::unique_ptr<char[]> buffer; + QuicByteCount frame_length = + HttpEncoder::SerializeMaxPushIdFrame(max_push_id_frame, &buffer); + return std::string(buffer.get(), frame_length); + } + QuicStreamId StreamCountToId(QuicStreamCount stream_count, Perspective perspective, bool bidirectional) { @@ -1770,6 +1779,55 @@ } } +TEST_P(QuicSpdySessionTestServer, ReduceMaxPushId) { + if (!VersionUsesHttp3(transport_version())) { + return; + } + + StrictMock<MockHttp3DebugVisitor> debug_visitor; + session_.set_debug_visitor(&debug_visitor); + + // Use an arbitrary stream id for incoming control stream. + QuicStreamId stream_id = + GetNthClientInitiatedUnidirectionalStreamId(transport_version(), 3); + char type[] = {kControlStream}; + quiche::QuicheStringPiece stream_type(type, 1); + + QuicStreamOffset offset = 0; + QuicStreamFrame data1(stream_id, false, offset, stream_type); + offset += stream_type.length(); + EXPECT_CALL(debug_visitor, OnPeerControlStreamCreated(stream_id)); + session_.OnStreamFrame(data1); + EXPECT_EQ(stream_id, + QuicSpdySessionPeer::GetReceiveControlStream(&session_)->id()); + + SettingsFrame settings; + std::string settings_frame = EncodeSettings(settings); + QuicStreamFrame data2(stream_id, false, offset, settings_frame); + offset += settings_frame.length(); + + EXPECT_CALL(debug_visitor, OnSettingsFrameReceived(settings)); + session_.OnStreamFrame(data2); + + std::string max_push_id_frame1 = SerializeMaxPushIdFrame(/* push_id = */ 3); + QuicStreamFrame data3(stream_id, false, offset, max_push_id_frame1); + offset += max_push_id_frame1.length(); + + EXPECT_CALL(debug_visitor, OnMaxPushIdFrameReceived(_)); + session_.OnStreamFrame(data3); + + std::string max_push_id_frame2 = SerializeMaxPushIdFrame(/* push_id = */ 1); + QuicStreamFrame data4(stream_id, false, offset, max_push_id_frame2); + + EXPECT_CALL(debug_visitor, OnMaxPushIdFrameReceived(_)); + EXPECT_CALL(*connection_, + CloseConnection(QUIC_HTTP_INVALID_MAX_PUSH_ID, + "MAX_PUSH_ID received with value 1 which is " + "smaller that previously received value 3", + _)); + session_.OnStreamFrame(data4); +} + class QuicSpdySessionTestClient : public QuicSpdySessionTestBase { protected: QuicSpdySessionTestClient()
diff --git a/quic/core/quic_error_codes.cc b/quic/core/quic_error_codes.cc index efbdc69..776ae8c 100644 --- a/quic/core/quic_error_codes.cc +++ b/quic/core/quic_error_codes.cc
@@ -198,6 +198,7 @@ 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_HTTP_INVALID_MAX_PUSH_ID); 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); @@ -557,6 +558,8 @@ static_cast<uint64_t>(QuicHttp3ErrorCode::MISSING_SETTINGS)}; case QUIC_HTTP_DUPLICATE_SETTING_IDENTIFIER: return {false, static_cast<uint64_t>(QuicHttp3ErrorCode::SETTINGS_ERROR)}; + case QUIC_HTTP_INVALID_MAX_PUSH_ID: + return {false, static_cast<uint64_t>(QuicHttp3ErrorCode::ID_ERROR)}; case QUIC_HPACK_INDEX_VARINT_ERROR: return {true, static_cast<uint64_t>(INTERNAL_ERROR)}; case QUIC_HPACK_NAME_LENGTH_VARINT_ERROR:
diff --git a/quic/core/quic_error_codes.h b/quic/core/quic_error_codes.h index b878de7..46e71b9 100644 --- a/quic/core/quic_error_codes.h +++ b/quic/core/quic_error_codes.h
@@ -426,6 +426,9 @@ QUIC_HTTP_MISSING_SETTINGS_FRAME = 157, // The received SETTINGS frame contains duplicate setting identifiers. QUIC_HTTP_DUPLICATE_SETTING_IDENTIFIER = 158, + // MAX_PUSH_ID frame received with push ID value smaller than a previously + // received value. + QUIC_HTTP_INVALID_MAX_PUSH_ID = 159, // HPACK header block decoding errors. // Index varint beyond implementation limit. @@ -462,7 +465,7 @@ QUIC_HPACK_COMPRESSED_HEADER_SIZE_EXCEEDS_LIMIT = 150, // No error. Used as bound while iterating. - QUIC_LAST_ERROR = 159, + QUIC_LAST_ERROR = 160, }; // 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