Access buffered CHLO in the correct place when the server changes the connection ID. Modified HTTP e2e tests to usually check the case where the connection ID case is 16B. Bugs are filed for test failures, then the tests edited to bypass those cases. PiperOrigin-RevId: 448307373
diff --git a/quiche/quic/core/http/end_to_end_test.cc b/quiche/quic/core/http/end_to_end_test.cc index 67d526d..019409d 100644 --- a/quiche/quic/core/http/end_to_end_test.cc +++ b/quiche/quic/core/http/end_to_end_test.cc
@@ -91,27 +91,39 @@ const char kBarResponseBody[] = "Palm hearts are pretty delicious, also."; const char kTestUserAgentId[] = "quic/core/http/end_to_end_test.cc"; const float kSessionToStreamRatio = 1.5; +const int kLongConnectionIdLength = 16; // Run all tests with the cross products of all versions. struct TestParams { - TestParams(const ParsedQuicVersion& version, QuicTag congestion_control_tag) - : version(version), congestion_control_tag(congestion_control_tag) {} + TestParams(const ParsedQuicVersion& version, QuicTag congestion_control_tag, + int override_server_connection_id_length) + : version(version), + congestion_control_tag(congestion_control_tag), + override_server_connection_id_length( + override_server_connection_id_length) {} friend std::ostream& operator<<(std::ostream& os, const TestParams& p) { os << "{ version: " << ParsedQuicVersionToString(p.version); os << " congestion_control_tag: " - << QuicTagToString(p.congestion_control_tag) << " }"; + << QuicTagToString(p.congestion_control_tag) + << " connection ID length: " << p.override_server_connection_id_length + << " }"; return os; } ParsedQuicVersion version; QuicTag congestion_control_tag; + int override_server_connection_id_length; }; // Used by ::testing::PrintToStringParamName(). std::string PrintToString(const TestParams& p) { - std::string rv = absl::StrCat(ParsedQuicVersionToString(p.version), "_", - QuicTagToString(p.congestion_control_tag)); + std::string rv = absl::StrCat( + ParsedQuicVersionToString(p.version), "_", + QuicTagToString(p.congestion_control_tag), "_", + std::to_string((p.override_server_connection_id_length == -1) + ? static_cast<int>(kQuicDefaultConnectionIdLength) + : p.override_server_connection_id_length)); std::replace(rv.begin(), rv.end(), ',', '_'); std::replace(rv.begin(), rv.end(), ' ', '_'); return rv; @@ -120,15 +132,27 @@ // Constructs various test permutations. std::vector<TestParams> GetTestParams() { std::vector<TestParams> params; - for (const QuicTag congestion_control_tag : {kRENO, kTBBR, kQBIC, kB2ON}) { - if (!GetQuicReloadableFlag(quic_allow_client_enabled_bbr_v2) && - congestion_control_tag == kB2ON) { - continue; - } - for (const ParsedQuicVersion& version : CurrentSupportedVersions()) { - params.push_back(TestParams(version, congestion_control_tag)); - } // End of outer version loop. - } // End of congestion_control_tag loop. + std::vector<int> connection_id_lengths{-1, kLongConnectionIdLength}; + for (auto connection_id_length : connection_id_lengths) { + for (const QuicTag congestion_control_tag : {kTBBR, kQBIC, kB2ON}) { + if (!GetQuicReloadableFlag(quic_allow_client_enabled_bbr_v2) && + congestion_control_tag == kB2ON) { + continue; + } + for (const ParsedQuicVersion& version : CurrentSupportedVersions()) { + // TODO(b/232269029): Q050 should be able to handle 0-RTT when the + // initial connection ID is > 8 bytes, but it cannot. This is an + // invasive fix that has no impact as long as gQUIC clients always use + // 8B server connection IDs. If this bug is fixed, we can change + // 'UsesTls' to 'AllowsVariableLengthConnectionIds()' below to test + // qQUIC as well. + if (connection_id_length == -1 || version.UsesTls()) { + params.push_back(TestParams(version, congestion_control_tag, + connection_id_length)); + } + } // End of outer version loop. + } // End of congestion_control_tag loop. + } // End of connection_id_length loop. return params; } @@ -182,6 +206,8 @@ server_supported_versions_(CurrentSupportedVersions()), chlo_multiplier_(0), stream_factory_(nullptr), + override_server_connection_id_length_( + GetParam().override_server_connection_id_length), expected_server_connection_id_length_(kQuicDefaultConnectionIdLength) { QUIC_LOG(INFO) << "Using Configuration: " << GetParam(); @@ -820,7 +846,7 @@ QuicTestServer::StreamFactory* stream_factory_; std::string pre_shared_key_client_; std::string pre_shared_key_server_; - int override_server_connection_id_length_ = -1; + int override_server_connection_id_length_; int override_client_connection_id_length_ = -1; uint8_t expected_server_connection_id_length_; bool enable_web_transport_ = false; @@ -1080,7 +1106,8 @@ } TEST_P(EndToEndTest, SimpleRequestResponseZeroConnectionID) { - if (!version_.AllowsVariableLengthConnectionIds()) { + if (!version_.AllowsVariableLengthConnectionIds() || + override_server_connection_id_length_ > -1) { ASSERT_TRUE(Initialize()); return; } @@ -1098,7 +1125,8 @@ } TEST_P(EndToEndTest, ZeroConnectionID) { - if (!version_.AllowsVariableLengthConnectionIds()) { + if (!version_.AllowsVariableLengthConnectionIds() || + override_server_connection_id_length_ > -1) { ASSERT_TRUE(Initialize()); return; } @@ -1114,7 +1142,8 @@ } TEST_P(EndToEndTest, BadConnectionIdLength) { - if (!version_.AllowsVariableLengthConnectionIds()) { + if (!version_.AllowsVariableLengthConnectionIds() || + override_server_connection_id_length_ > -1) { ASSERT_TRUE(Initialize()); return; } @@ -1128,23 +1157,6 @@ .length()); } -// Tests a very long (16-byte) initial destination connection ID to make -// sure the dispatcher properly replaces it with an 8-byte one. -TEST_P(EndToEndTest, LongBadConnectionIdLength) { - if (!version_.AllowsVariableLengthConnectionIds()) { - ASSERT_TRUE(Initialize()); - return; - } - override_server_connection_id_length_ = 16; - ASSERT_TRUE(Initialize()); - SendSynchronousFooRequestAndCheckResponse(); - EXPECT_EQ(kQuicDefaultConnectionIdLength, client_->client() - ->client_session() - ->connection() - ->connection_id() - .length()); -} - TEST_P(EndToEndTest, ClientConnectionId) { if (!version_.SupportsClientConnectionIds()) { ASSERT_TRUE(Initialize()); @@ -1179,7 +1191,8 @@ } TEST_P(EndToEndTest, ForcedVersionNegotiationAndBadConnectionIdLength) { - if (!version_.AllowsVariableLengthConnectionIds()) { + if (!version_.AllowsVariableLengthConnectionIds() || + override_server_connection_id_length_ > -1) { ASSERT_TRUE(Initialize()); return; } @@ -1200,13 +1213,13 @@ // connection ID. TEST_P(EndToEndTest, ForcedVersNegoAndClientCIDAndLongCID) { if (!version_.SupportsClientConnectionIds() || - !version_.AllowsVariableLengthConnectionIds()) { + !version_.AllowsVariableLengthConnectionIds() || + override_server_connection_id_length_ != kLongConnectionIdLength) { ASSERT_TRUE(Initialize()); return; } client_supported_versions_.insert(client_supported_versions_.begin(), QuicVersionReservedForNegotiation()); - override_server_connection_id_length_ = 16; override_client_connection_id_length_ = 18; ASSERT_TRUE(Initialize()); ASSERT_TRUE(ServerSendsVersionNegotiation()); @@ -1224,7 +1237,8 @@ } TEST_P(EndToEndTest, MixGoodAndBadConnectionIdLengths) { - if (!version_.AllowsVariableLengthConnectionIds()) { + if (!version_.AllowsVariableLengthConnectionIds() || + override_server_connection_id_length_ > -1) { ASSERT_TRUE(Initialize()); return; } @@ -1372,7 +1386,8 @@ } TEST_P(EndToEndTest, MultipleRequestResponseZeroConnectionID) { - if (!version_.AllowsVariableLengthConnectionIds()) { + if (!version_.AllowsVariableLengthConnectionIds() || + override_server_connection_id_length_ > -1) { ASSERT_TRUE(Initialize()); return; } @@ -1531,7 +1546,14 @@ EXPECT_EQ(kFooResponseBody, client_->SendCustomSynchronousRequest(headers, body)); - VerifyCleanConnection(true); + if (override_server_connection_id_length_ == -1) { + // If the client sends a longer connection ID, we can end up with dropped + // packets. The packets_dropped counter increments whenever a packet arrives + // with a new server connection ID that is not INITIAL, RETRY, or 1-RTT. + // With packet losses, we could easily lose a server INITIAL and have the + // first observed server packet be HANDSHAKE. + VerifyCleanConnection(true); + } } // Regression test for b/80090281. @@ -4774,6 +4796,14 @@ // Regression test for b/116200989. TEST_P(EndToEndTest, SendStatelessResetIfServerConnectionClosedLocallyDuringHandshake) { + // TODO(b/232285861) + // This test should probably work when the server issues a new connection ID + // during the handshake. When the bug is fixed, remove the check below to + // allow it to run when the client provides a 16B connection ID. + if (override_server_connection_id_length_ > -1) { + ASSERT_TRUE(Initialize()); + return; + } connect_to_server_on_initialize_ = false; ASSERT_TRUE(Initialize()); @@ -5848,7 +5878,8 @@ } TEST_P(EndToEndTest, LegacyVersionEncapsulation) { - if (!version_.HasLongHeaderLengths()) { + if (!version_.HasLongHeaderLengths() || + override_server_connection_id_length_ > -1) { // Decapsulating Legacy Version Encapsulation packets from these versions // is not currently supported in QuicDispatcher. ASSERT_TRUE(Initialize()); @@ -5865,7 +5896,8 @@ } TEST_P(EndToEndTest, LegacyVersionEncapsulationWithMultiPacketChlo) { - if (!version_.HasLongHeaderLengths()) { + if (!version_.HasLongHeaderLengths() || + override_server_connection_id_length_ > -1) { // Decapsulating Legacy Version Encapsulation packets from these versions // is not currently supported in QuicDispatcher. ASSERT_TRUE(Initialize()); @@ -5892,7 +5924,8 @@ } TEST_P(EndToEndTest, LegacyVersionEncapsulationWithVersionNegotiation) { - if (!version_.HasLongHeaderLengths()) { + if (!version_.HasLongHeaderLengths() || + override_server_connection_id_length_ > -1) { // Decapsulating Legacy Version Encapsulation packets from these versions // is not currently supported in QuicDispatcher. ASSERT_TRUE(Initialize()); @@ -5911,7 +5944,8 @@ } TEST_P(EndToEndTest, LegacyVersionEncapsulationWithLoss) { - if (!version_.HasLongHeaderLengths()) { + if (!version_.HasLongHeaderLengths() || + override_server_connection_id_length_ > -1) { // Decapsulating Legacy Version Encapsulation packets from these versions // is not currently supported in QuicDispatcher. ASSERT_TRUE(Initialize()); @@ -5974,7 +6008,7 @@ // Parse the saved packet to make sure it's valid. SimpleQuicFramer validation_framer({version_}); validation_framer.framer()->SetInitialObfuscators( - GetClientConnection()->connection_id()); + GetServerConnection()->GetOriginalDestinationConnectionId()); ASSERT_GT(copying_writer->packets().size(), 0u); EXPECT_TRUE(validation_framer.ProcessPacket(*copying_writer->packets()[0])); // TODO(dschinazi) figure out a way to use a MockRandom in this test so we
diff --git a/quiche/quic/core/quic_dispatcher.cc b/quiche/quic/core/quic_dispatcher.cc index 38a3cb6..072246d 100644 --- a/quiche/quic/core/quic_dispatcher.cc +++ b/quiche/quic/core/quic_dispatcher.cc
@@ -1321,7 +1321,9 @@ << " ALPN \"" << alpn << "\" version " << packet_info->version; return; } - if (original_connection_id != packet_info->destination_connection_id) { + const bool replaced_connection_id = + original_connection_id != packet_info->destination_connection_id; + if (replaced_connection_id) { session->connection()->SetOriginalDestinationConnectionId( original_connection_id); } @@ -1342,8 +1344,11 @@ } session_ptr = insertion_result.first->second.get(); std::list<BufferedPacket> packets = - buffered_packets_.DeliverPackets(packet_info->destination_connection_id) - .buffered_packets; + buffered_packets_.DeliverPackets(original_connection_id).buffered_packets; + if (replaced_connection_id && !packets.empty()) { + QUIC_CODE_COUNT( + quic_delivered_buffered_packets_to_connection_with_replaced_id); + } // Process CHLO at first. session_ptr->ProcessUdpPacket(packet_info->self_address, packet_info->peer_address, packet_info->packet);
diff --git a/quiche/quic/core/quic_dispatcher_test.cc b/quiche/quic/core/quic_dispatcher_test.cc index 40847ff..3f65d3e 100644 --- a/quiche/quic/core/quic_dispatcher_test.cc +++ b/quiche/quic/core/quic_dispatcher_test.cc
@@ -490,7 +490,8 @@ ProcessFirstFlight(version, client_address, connection_id); } - void TestTlsMultiPacketClientHello(bool add_reordering); + void TestTlsMultiPacketClientHello(bool add_reordering, + bool long_connection_id); void TestVersionNegotiationForUnknownVersionInvalidShortInitialConnectionId( const QuicConnectionId& server_connection_id, @@ -560,14 +561,23 @@ } void QuicDispatcherTestBase::TestTlsMultiPacketClientHello( - bool add_reordering) { + bool add_reordering, bool long_connection_id) { if (!version_.UsesTls()) { return; } SetAddressToken("857293462398"); QuicSocketAddress client_address(QuicIpAddress::Loopback4(), 1); - QuicConnectionId server_connection_id = TestConnectionId(); + QuicConnectionId original_connection_id, new_connection_id; + if (long_connection_id) { + original_connection_id = QuicConnectionId( + {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09}); + new_connection_id = + QuicConnectionId({0x6c, 0x6b, 0x4b, 0xad, 0x8d, 0x00, 0x24, 0xd8}); + } else { + original_connection_id = TestConnectionId(); + new_connection_id = original_connection_id; + } QuicConfig client_config = DefaultQuicConfig(); // Add a 2000-byte custom parameter to increase the length of the CHLO. constexpr auto kCustomParameterId = @@ -576,7 +586,7 @@ client_config.custom_transport_parameters_to_send()[kCustomParameterId] = kCustomParameterValue; std::vector<std::unique_ptr<QuicReceivedPacket>> packets = - GetFirstFlightOfPackets(version_, client_config, server_connection_id, + GetFirstFlightOfPackets(version_, client_config, original_connection_id, EmptyQuicConnectionId(), TestClientCryptoConfig()); ASSERT_EQ(packets.size(), 2u); @@ -585,11 +595,12 @@ } // Processing the first packet should not create a new session. - EXPECT_CALL(*dispatcher_, - ShouldCreateOrBufferPacketForConnection( - ReceivedPacketInfoConnectionIdEquals(server_connection_id))); + EXPECT_CALL( + *dispatcher_, + ShouldCreateOrBufferPacketForConnection( + ReceivedPacketInfoConnectionIdEquals(original_connection_id))); ProcessReceivedPacket(std::move(packets[0]), client_address, version_, - server_connection_id); + original_connection_id); EXPECT_EQ(dispatcher_->NumSessions(), 0u) << "No session should be created before the rest of the CHLO arrives."; @@ -597,10 +608,10 @@ // Processing the second packet should create the new session. EXPECT_CALL( *dispatcher_, - CreateQuicSession(server_connection_id, _, client_address, + CreateQuicSession(new_connection_id, _, client_address, Eq(ExpectedAlpn()), _, Eq(ParsedClientHelloForTest()))) .WillOnce(Return(ByMove(CreateSession( - dispatcher_.get(), config_, server_connection_id, client_address, + dispatcher_.get(), config_, new_connection_id, client_address, &mock_helper_, &mock_alarm_factory_, &crypto_config_, QuicDispatcherPeer::GetCache(dispatcher_.get()), &session1_)))); EXPECT_CALL(*reinterpret_cast<MockQuicConnection*>(session1_->connection()), @@ -608,16 +619,29 @@ .Times(2); ProcessReceivedPacket(std::move(packets[1]), client_address, version_, - server_connection_id); + original_connection_id); EXPECT_EQ(dispatcher_->NumSessions(), 1u); } TEST_P(QuicDispatcherTestAllVersions, TlsMultiPacketClientHello) { - TestTlsMultiPacketClientHello(/*add_reordering=*/false); + TestTlsMultiPacketClientHello(/*add_reordering=*/false, + /*long_connection_id=*/false); } TEST_P(QuicDispatcherTestAllVersions, TlsMultiPacketClientHelloWithReordering) { - TestTlsMultiPacketClientHello(/*add_reordering=*/true); + TestTlsMultiPacketClientHello(/*add_reordering=*/true, + /*long_connection_id=*/false); +} + +TEST_P(QuicDispatcherTestAllVersions, TlsMultiPacketClientHelloWithLongId) { + TestTlsMultiPacketClientHello(/*add_reordering=*/false, + /*long_connection_id=*/true); +} + +TEST_P(QuicDispatcherTestAllVersions, + TlsMultiPacketClientHelloWithReorderingAndLongId) { + TestTlsMultiPacketClientHello(/*add_reordering=*/true, + /*long_connection_id=*/true); } TEST_P(QuicDispatcherTestAllVersions, LegacyVersionEncapsulation) {