Fix expected_server_connection_id_length on client
When should_update_expected_server_connection_id_length is true, ProcessAndValidateIetfConnectionIdLength, expected_server_connection_id_length will update expected_server_connection_id_length. Unfortunately that code was wrong on the client as it only looked at the destination connection ID length instead of the source on the client. This CL fixes that error and adds a test. This wasn't an issue in production because we've only ever used should_update_expected_server_connection_id_length on the server.
gfe-relnote: client-only change, not flag protected
PiperOrigin-RevId: 250586674
Change-Id: Iccb6776bc9aa41350f1e80ecddea76ba8aa4eb30
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc
index 7e34b3b..328e453 100644
--- a/quic/core/quic_framer.cc
+++ b/quic/core/quic_framer.cc
@@ -2624,6 +2624,7 @@
bool QuicFramer::ProcessAndValidateIetfConnectionIdLength(
QuicDataReader* reader,
ParsedQuicVersion version,
+ Perspective perspective,
bool should_update_expected_server_connection_id_length,
uint8_t* expected_server_connection_id_length,
uint8_t* destination_connection_id_length,
@@ -2639,17 +2640,20 @@
if (dcil != 0) {
dcil += kConnectionIdLengthAdjustment;
}
- if (should_update_expected_server_connection_id_length &&
- *expected_server_connection_id_length != dcil) {
- QUIC_DVLOG(1) << "Updating expected_server_connection_id_length: "
- << static_cast<int>(*expected_server_connection_id_length)
- << " -> " << static_cast<int>(dcil);
- *expected_server_connection_id_length = dcil;
- }
uint8_t scil = connection_id_lengths_byte & kSourceConnectionIdLengthMask;
if (scil != 0) {
scil += kConnectionIdLengthAdjustment;
}
+ if (should_update_expected_server_connection_id_length) {
+ uint8_t server_connection_id_length =
+ perspective == Perspective::IS_SERVER ? dcil : scil;
+ if (*expected_server_connection_id_length != server_connection_id_length) {
+ QUIC_DVLOG(1) << "Updating expected_server_connection_id_length: "
+ << static_cast<int>(*expected_server_connection_id_length)
+ << " -> " << static_cast<int>(server_connection_id_length);
+ *expected_server_connection_id_length = server_connection_id_length;
+ }
+ }
if (!should_update_expected_server_connection_id_length &&
(dcil != *destination_connection_id_length ||
scil != *source_connection_id_length) &&
@@ -2683,7 +2687,7 @@
: 0;
if (header->form == IETF_QUIC_LONG_HEADER_PACKET) {
if (!ProcessAndValidateIetfConnectionIdLength(
- reader, header->version,
+ reader, header->version, perspective_,
should_update_expected_server_connection_id_length_,
&expected_server_connection_id_length_,
&destination_connection_id_length, &source_connection_id_length,
@@ -6099,6 +6103,7 @@
uint8_t unused_expected_server_connection_id_length = 0;
if (!ProcessAndValidateIetfConnectionIdLength(
&reader, ParseQuicVersionLabel(*version_label),
+ Perspective::IS_SERVER,
/*should_update_expected_server_connection_id_length=*/true,
&unused_expected_server_connection_id_length,
destination_connection_id_length,
@@ -6247,7 +6252,7 @@
uint8_t expected_server_connection_id_length = 0,
destination_connection_id_length = 0, source_connection_id_length = 0;
if (!ProcessAndValidateIetfConnectionIdLength(
- &reader, UnsupportedQuicVersion(),
+ &reader, UnsupportedQuicVersion(), Perspective::IS_CLIENT,
/*should_update_expected_server_connection_id_length=*/true,
&expected_server_connection_id_length,
&destination_connection_id_length, &source_connection_id_length,
diff --git a/quic/core/quic_framer.h b/quic/core/quic_framer.h
index 11d48bb..e6eecf1 100644
--- a/quic/core/quic_framer.h
+++ b/quic/core/quic_framer.h
@@ -714,10 +714,15 @@
QuicVersionLabel* version_label);
// Validates and updates |destination_connection_id_length| and
- // |source_connection_id_length|.
+ // |source_connection_id_length|. When
+ // |should_update_expected_server_connection_id_length| is true, length
+ // validation is disabled and |expected_server_connection_id_length| is set
+ // to the appropriate length.
+ // TODO(b/133873272) refactor this method.
static bool ProcessAndValidateIetfConnectionIdLength(
QuicDataReader* reader,
ParsedQuicVersion version,
+ Perspective perspective,
bool should_update_expected_server_connection_id_length,
uint8_t* expected_server_connection_id_length,
uint8_t* destination_connection_id_length,
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc
index 13e4482..22ffe4f 100644
--- a/quic/core/quic_framer_test.cc
+++ b/quic/core/quic_framer_test.cc
@@ -13840,6 +13840,82 @@
}
}
+TEST_P(QuicFramerTest, ProcessAndValidateIetfConnectionIdLengthClient) {
+ if (framer_.transport_version() <= QUIC_VERSION_43) {
+ // This test requires an IETF long header.
+ return;
+ }
+ char connection_id_lengths = 0x05;
+ QuicDataReader reader(&connection_id_lengths, 1);
+
+ bool should_update_expected_server_connection_id_length = false;
+ uint8_t expected_server_connection_id_length = 8;
+ uint8_t destination_connection_id_length = 0;
+ uint8_t source_connection_id_length = 8;
+ std::string detailed_error = "";
+
+ EXPECT_TRUE(QuicFramerPeer::ProcessAndValidateIetfConnectionIdLength(
+ &reader, framer_.version(), Perspective::IS_CLIENT,
+ should_update_expected_server_connection_id_length,
+ &expected_server_connection_id_length, &destination_connection_id_length,
+ &source_connection_id_length, &detailed_error));
+ EXPECT_EQ(8, expected_server_connection_id_length);
+ EXPECT_EQ(0, destination_connection_id_length);
+ EXPECT_EQ(8, source_connection_id_length);
+ EXPECT_EQ("", detailed_error);
+
+ QuicDataReader reader2(&connection_id_lengths, 1);
+ should_update_expected_server_connection_id_length = true;
+ expected_server_connection_id_length = 33;
+ EXPECT_TRUE(QuicFramerPeer::ProcessAndValidateIetfConnectionIdLength(
+ &reader2, framer_.version(), Perspective::IS_CLIENT,
+ should_update_expected_server_connection_id_length,
+ &expected_server_connection_id_length, &destination_connection_id_length,
+ &source_connection_id_length, &detailed_error));
+ EXPECT_EQ(8, expected_server_connection_id_length);
+ EXPECT_EQ(0, destination_connection_id_length);
+ EXPECT_EQ(8, source_connection_id_length);
+ EXPECT_EQ("", detailed_error);
+}
+
+TEST_P(QuicFramerTest, ProcessAndValidateIetfConnectionIdLengthServer) {
+ if (framer_.transport_version() <= QUIC_VERSION_43) {
+ // This test requires an IETF long header.
+ return;
+ }
+ char connection_id_lengths = 0x50;
+ QuicDataReader reader(&connection_id_lengths, 1);
+
+ bool should_update_expected_server_connection_id_length = false;
+ uint8_t expected_server_connection_id_length = 8;
+ uint8_t destination_connection_id_length = 8;
+ uint8_t source_connection_id_length = 0;
+ std::string detailed_error = "";
+
+ EXPECT_TRUE(QuicFramerPeer::ProcessAndValidateIetfConnectionIdLength(
+ &reader, framer_.version(), Perspective::IS_SERVER,
+ should_update_expected_server_connection_id_length,
+ &expected_server_connection_id_length, &destination_connection_id_length,
+ &source_connection_id_length, &detailed_error));
+ EXPECT_EQ(8, expected_server_connection_id_length);
+ EXPECT_EQ(8, destination_connection_id_length);
+ EXPECT_EQ(0, source_connection_id_length);
+ EXPECT_EQ("", detailed_error);
+
+ QuicDataReader reader2(&connection_id_lengths, 1);
+ should_update_expected_server_connection_id_length = true;
+ expected_server_connection_id_length = 33;
+ EXPECT_TRUE(QuicFramerPeer::ProcessAndValidateIetfConnectionIdLength(
+ &reader2, framer_.version(), Perspective::IS_SERVER,
+ should_update_expected_server_connection_id_length,
+ &expected_server_connection_id_length, &destination_connection_id_length,
+ &source_connection_id_length, &detailed_error));
+ EXPECT_EQ(8, expected_server_connection_id_length);
+ EXPECT_EQ(8, destination_connection_id_length);
+ EXPECT_EQ(0, source_connection_id_length);
+ EXPECT_EQ("", detailed_error);
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/test_tools/quic_framer_peer.cc b/quic/test_tools/quic_framer_peer.cc
index 6eedd60..94493c3 100644
--- a/quic/test_tools/quic_framer_peer.cc
+++ b/quic/test_tools/quic_framer_peer.cc
@@ -351,5 +351,22 @@
return framer->largest_decrypted_packet_numbers_[packet_number_space];
}
+// static
+bool QuicFramerPeer::ProcessAndValidateIetfConnectionIdLength(
+ QuicDataReader* reader,
+ ParsedQuicVersion version,
+ Perspective perspective,
+ bool should_update_expected_server_connection_id_length,
+ uint8_t* expected_server_connection_id_length,
+ uint8_t* destination_connection_id_length,
+ uint8_t* source_connection_id_length,
+ std::string* detailed_error) {
+ return QuicFramer::ProcessAndValidateIetfConnectionIdLength(
+ reader, version, perspective,
+ should_update_expected_server_connection_id_length,
+ expected_server_connection_id_length, destination_connection_id_length,
+ source_connection_id_length, detailed_error);
+}
+
} // namespace test
} // namespace quic
diff --git a/quic/test_tools/quic_framer_peer.h b/quic/test_tools/quic_framer_peer.h
index 0b17c19..7a09a92 100644
--- a/quic/test_tools/quic_framer_peer.h
+++ b/quic/test_tools/quic_framer_peer.h
@@ -166,6 +166,16 @@
static QuicPacketNumber GetLargestDecryptedPacketNumber(
QuicFramer* framer,
PacketNumberSpace packet_number_space);
+
+ static bool ProcessAndValidateIetfConnectionIdLength(
+ QuicDataReader* reader,
+ ParsedQuicVersion version,
+ Perspective perspective,
+ bool should_update_expected_server_connection_id_length,
+ uint8_t* expected_server_connection_id_length,
+ uint8_t* destination_connection_id_length,
+ uint8_t* source_connection_id_length,
+ std::string* detailed_error);
};
} // namespace test
