gfe-relnote: QUIC_BUG instead of LOG(ERROR) in CreateQuicVersionLabel if the HandshakeProtocol is unknown QuicFramer::ProcessVersionNegotiationPacket also filters out unknown versions before saving them to the QuicVersionNegotiationPacket struct. PiperOrigin-RevId: 251515152 Change-Id: I8b4ed34ff59e1760051e42b5659e5acc987d7ee3
diff --git a/quic/core/quic_dispatcher.cc b/quic/core/quic_dispatcher.cc index 1611287..317f469 100644 --- a/quic/core/quic_dispatcher.cc +++ b/quic/core/quic_dispatcher.cc
@@ -844,7 +844,7 @@ termination_packets.push_back(QuicFramer::BuildVersionNegotiationPacket( server_connection_id, EmptyQuicConnectionId(), /*ietf_quic=*/format != GOOGLE_QUIC_PACKET, - ParsedQuicVersionVector{UnsupportedQuicVersion()})); + ParsedQuicVersionVector{QuicVersionReservedForNegotiation()})); if (format == GOOGLE_QUIC_PACKET) { QUIC_RELOADABLE_FLAG_COUNT_N(quic_terminate_gquic_connection_as_ietf, 2, 2); }
diff --git a/quic/core/quic_dispatcher_test.cc b/quic/core/quic_dispatcher_test.cc index 1fb11ce..e6936ab 100644 --- a/quic/core/quic_dispatcher_test.cc +++ b/quic/core/quic_dispatcher_test.cc
@@ -487,14 +487,12 @@ EXPECT_CALL(*time_wait_list_manager_, SendVersionNegotiationPacket(_, _, _, _, _, _, _)) .Times(1); - QuicTransportVersion version = - static_cast<QuicTransportVersion>(QuicTransportVersionMin() - 1); - ParsedQuicVersion parsed_version(PROTOCOL_QUIC_CRYPTO, version); // Pad the CHLO message with enough data to make the packet large enough // to trigger version negotiation. std::string chlo = SerializeCHLO() + std::string(1200, 'a'); DCHECK_LE(1200u, chlo.length()); - ProcessPacket(client_address, TestConnectionId(1), true, parsed_version, chlo, + ProcessPacket(client_address, TestConnectionId(1), true, + QuicVersionReservedForNegotiation(), chlo, CONNECTION_ID_PRESENT, PACKET_4BYTE_PACKET_NUMBER, 1); } @@ -506,18 +504,15 @@ EXPECT_CALL(*time_wait_list_manager_, SendVersionNegotiationPacket(_, _, _, _, _, _, _)) .Times(0); - QuicTransportVersion version = - static_cast<QuicTransportVersion>(QuicTransportVersionMin() - 1); - ParsedQuicVersion parsed_version(PROTOCOL_QUIC_CRYPTO, version); std::string chlo = SerializeCHLO() + std::string(1200, 'a'); // Truncate to 1100 bytes of payload which results in a packet just // under 1200 bytes after framing, packet, and encryption overhead. DCHECK_LE(1200u, chlo.length()); std::string truncated_chlo = chlo.substr(0, 1100); DCHECK_EQ(1100u, truncated_chlo.length()); - ProcessPacket(client_address, TestConnectionId(1), true, parsed_version, - truncated_chlo, CONNECTION_ID_PRESENT, - PACKET_4BYTE_PACKET_NUMBER, 1); + ProcessPacket(client_address, TestConnectionId(1), true, + QuicVersionReservedForNegotiation(), truncated_chlo, + CONNECTION_ID_PRESENT, PACKET_4BYTE_PACKET_NUMBER, 1); } // Disabling CHLO size validation allows the dispatcher to send version @@ -532,18 +527,15 @@ EXPECT_CALL(*time_wait_list_manager_, SendVersionNegotiationPacket(_, _, _, _, _, _, _)) .Times(1); - QuicTransportVersion version = - static_cast<QuicTransportVersion>(QuicTransportVersionMin() - 1); - ParsedQuicVersion parsed_version(PROTOCOL_QUIC_CRYPTO, version); std::string chlo = SerializeCHLO() + std::string(1200, 'a'); // Truncate to 1100 bytes of payload which results in a packet just // under 1200 bytes after framing, packet, and encryption overhead. DCHECK_LE(1200u, chlo.length()); std::string truncated_chlo = chlo.substr(0, 1100); DCHECK_EQ(1100u, truncated_chlo.length()); - ProcessPacket(client_address, TestConnectionId(1), true, parsed_version, - truncated_chlo, CONNECTION_ID_PRESENT, - PACKET_4BYTE_PACKET_NUMBER, 1); + ProcessPacket(client_address, TestConnectionId(1), true, + QuicVersionReservedForNegotiation(), truncated_chlo, + CONNECTION_ID_PRESENT, PACKET_4BYTE_PACKET_NUMBER, 1); } TEST_F(QuicDispatcherTest, Shutdown) { @@ -856,10 +848,8 @@ EXPECT_CALL(*dispatcher_, CreateQuicSession(connection_id, client_address, QuicStringPiece("hq"), _)) .Times(0); - ParsedQuicVersion version( - PROTOCOL_QUIC_CRYPTO, - static_cast<QuicTransportVersion>(QuicTransportVersionMin() - 1)); - ProcessPacket(client_address, connection_id, true, version, SerializeCHLO(), + ProcessPacket(client_address, connection_id, true, + QuicVersionReservedForNegotiation(), SerializeCHLO(), CONNECTION_ID_PRESENT, PACKET_4BYTE_PACKET_NUMBER, 1); connection_id = TestConnectionId(++conn_id); EXPECT_CALL(*dispatcher_, CreateQuicSession(connection_id, client_address,
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc index df019cf..27237d7 100644 --- a/quic/core/quic_framer.cc +++ b/quic/core/quic_framer.cc
@@ -1572,7 +1572,10 @@ DroppedPacketReason::INVALID_VERSION_NEGOTIATION_PACKET); return RaiseError(QUIC_INVALID_VERSION_NEGOTIATION_PACKET); } - packet.versions.push_back(ParseQuicVersionLabel(version_label)); + ParsedQuicVersion parsed_version = ParseQuicVersionLabel(version_label); + if (parsed_version != UnsupportedQuicVersion()) { + packet.versions.push_back(parsed_version); + } } while (!reader->IsDoneReading()); QUIC_DLOG(INFO) << ENDPOINT << "parsed version negotiation: "
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc index 6d2dd8e..6890c85 100644 --- a/quic/core/quic_framer_test.cc +++ b/quic/core/quic_framer_test.cc
@@ -4751,7 +4751,7 @@ EXPECT_TRUE(framer_.ProcessPacket(*encrypted)); ASSERT_EQ(QUIC_NO_ERROR, framer_.error()); ASSERT_TRUE(visitor_.version_negotiation_packet_.get()); - EXPECT_EQ(2u, visitor_.version_negotiation_packet_->versions.size()); + EXPECT_EQ(1u, visitor_.version_negotiation_packet_->versions.size()); EXPECT_EQ(GetParam(), visitor_.version_negotiation_packet_->versions[0]); // Remove the last version from the packet so that every truncated @@ -4823,7 +4823,7 @@ EXPECT_TRUE(framer_.ProcessPacket(*encrypted)); ASSERT_EQ(QUIC_NO_ERROR, framer_.error()); ASSERT_TRUE(visitor_.version_negotiation_packet_.get()); - EXPECT_EQ(2u, visitor_.version_negotiation_packet_->versions.size()); + EXPECT_EQ(1u, visitor_.version_negotiation_packet_->versions.size()); EXPECT_EQ(GetParam(), visitor_.version_negotiation_packet_->versions[0]); // Remove the last version from the packet so that every truncated
diff --git a/quic/core/quic_versions.cc b/quic/core/quic_versions.cc index 0d2d2db..e23fc53 100644 --- a/quic/core/quic_versions.cc +++ b/quic/core/quic_versions.cc
@@ -71,8 +71,8 @@ proto = 'T'; break; default: - QUIC_LOG(ERROR) << "Invalid HandshakeProtocol: " - << parsed_version.handshake_protocol; + QUIC_BUG << "Invalid HandshakeProtocol: " + << parsed_version.handshake_protocol; return 0; } switch (parsed_version.transport_version) { @@ -94,10 +94,10 @@ case QUIC_VERSION_RESERVED_FOR_NEGOTIATION: return MakeVersionLabel(0xda, 0x5a, 0x3a, 0x3a); default: - // This shold be an ERROR because we should never attempt to convert an - // invalid QuicTransportVersion to be written to the wire. - QUIC_LOG(ERROR) << "Unsupported QuicTransportVersion: " - << parsed_version.transport_version; + // This is a bug because we should never attempt to convert an invalid + // QuicTransportVersion to be written to the wire. + QUIC_BUG << "Unsupported QuicTransportVersion: " + << parsed_version.transport_version; return 0; } } @@ -341,6 +341,9 @@ } std::string ParsedQuicVersionToString(ParsedQuicVersion version) { + if (version == UnsupportedQuicVersion()) { + return "0"; + } return QuicVersionLabelToString(CreateQuicVersionLabel(version)); }
diff --git a/quic/core/quic_versions_test.cc b/quic/core/quic_versions_test.cc index fbd7a03..0b719d5 100644 --- a/quic/core/quic_versions_test.cc +++ b/quic/core/quic_versions_test.cc
@@ -5,6 +5,7 @@ #include "net/third_party/quiche/src/quic/core/quic_versions.h" #include "net/third_party/quiche/src/quic/platform/api/quic_arraysize.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_expect_bug.h" #include "net/third_party/quiche/src/quic/platform/api/quic_flags.h" #include "net/third_party/quiche/src/quic/platform/api/quic_logging.h" #include "net/third_party/quiche/src/quic/platform/api/quic_mock_log.h" @@ -47,18 +48,8 @@ } TEST_F(QuicVersionsTest, QuicVersionToQuicVersionLabelUnsupported) { - // TODO(rjshade): Change to DFATAL once we actually support multiple versions, - // and QuicConnectionTest::SendVersionNegotiationPacket can be changed to use - // mis-matched versions rather than relying on QUIC_VERSION_UNSUPPORTED. - CREATE_QUIC_MOCK_LOG(log); - log.StartCapturingLogs(); - - if (QUIC_LOG_ERROR_IS_ON()) { - EXPECT_QUIC_LOG_CALL_CONTAINS(log, ERROR, - "Unsupported QuicTransportVersion: 0"); - } - - EXPECT_EQ(0u, QuicVersionToQuicVersionLabel(QUIC_VERSION_UNSUPPORTED)); + EXPECT_QUIC_BUG(QuicVersionToQuicVersionLabel(QUIC_VERSION_UNSUPPORTED), + "Unsupported QuicTransportVersion: 0"); } TEST_F(QuicVersionsTest, QuicVersionLabelToQuicTransportVersion) {