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) {
