gfe-relnote: In QUIC, enforce packets be ACKed in the correct packet number space. Protected by enabling multiple packet number spaces. PiperOrigin-RevId: 243853453 Change-Id: Iee8f806247f85ed784f4872f380f334754bc7e22
diff --git a/quic/core/congestion_control/general_loss_algorithm_test.cc b/quic/core/congestion_control/general_loss_algorithm_test.cc index 0172dfc..a249679 100644 --- a/quic/core/congestion_control/general_loss_algorithm_test.cc +++ b/quic/core/congestion_control/general_loss_algorithm_test.cc
@@ -59,7 +59,7 @@ const std::vector<uint64_t>& losses_expected) { if (unacked_packets_.use_uber_loss_algorithm()) { unacked_packets_.MaybeUpdateLargestAckedOfPacketNumberSpace( - ENCRYPTION_INITIAL, QuicPacketNumber(largest_newly_acked)); + APPLICATION_DATA, QuicPacketNumber(largest_newly_acked)); } else if (!unacked_packets_.largest_acked().IsInitialized() || QuicPacketNumber(largest_newly_acked) > unacked_packets_.largest_acked()) { @@ -217,7 +217,7 @@ // Early retransmit when the final packet gets acked and the first is nacked. if (unacked_packets_.use_uber_loss_algorithm()) { unacked_packets_.MaybeUpdateLargestAckedOfPacketNumberSpace( - ENCRYPTION_INITIAL, QuicPacketNumber(2)); + APPLICATION_DATA, QuicPacketNumber(2)); } else { unacked_packets_.IncreaseLargestAcked(QuicPacketNumber(2)); } @@ -240,7 +240,7 @@ // Early retransmit when the final packet gets acked and the first is nacked. if (unacked_packets_.use_uber_loss_algorithm()) { unacked_packets_.MaybeUpdateLargestAckedOfPacketNumberSpace( - ENCRYPTION_INITIAL, QuicPacketNumber(2)); + APPLICATION_DATA, QuicPacketNumber(2)); } else { unacked_packets_.IncreaseLargestAcked(QuicPacketNumber(2)); } @@ -272,7 +272,7 @@ clock_.AdvanceTime(rtt_stats_.smoothed_rtt()); if (unacked_packets_.use_uber_loss_algorithm()) { unacked_packets_.MaybeUpdateLargestAckedOfPacketNumberSpace( - ENCRYPTION_INITIAL, QuicPacketNumber(2)); + APPLICATION_DATA, QuicPacketNumber(2)); } else { unacked_packets_.IncreaseLargestAcked(QuicPacketNumber(2)); }
diff --git a/quic/core/congestion_control/uber_loss_algorithm_test.cc b/quic/core/congestion_control/uber_loss_algorithm_test.cc index 2945214..33b9393 100644 --- a/quic/core/congestion_control/uber_loss_algorithm_test.cc +++ b/quic/core/congestion_control/uber_loss_algorithm_test.cc
@@ -90,7 +90,7 @@ AckPackets({1, 4}); unacked_packets_->MaybeUpdateLargestAckedOfPacketNumberSpace( - ENCRYPTION_INITIAL, QuicPacketNumber(4)); + HANDSHAKE_DATA, QuicPacketNumber(4)); // Verify no packet is detected lost. VerifyLosses(4, packets_acked_, std::vector<uint64_t>{}); EXPECT_EQ(QuicTime::Zero(), loss_algorithm_.GetLossTimeout()); @@ -106,7 +106,7 @@ AckPackets({4}); unacked_packets_->MaybeUpdateLargestAckedOfPacketNumberSpace( - ENCRYPTION_ZERO_RTT, QuicPacketNumber(4)); + APPLICATION_DATA, QuicPacketNumber(4)); // No packet loss by acking 4. VerifyLosses(4, packets_acked_, std::vector<uint64_t>{}); EXPECT_EQ(QuicTime::Zero(), loss_algorithm_.GetLossTimeout()); @@ -114,7 +114,7 @@ // Acking 6 causes 3 to be detected loss. AckPackets({6}); unacked_packets_->MaybeUpdateLargestAckedOfPacketNumberSpace( - ENCRYPTION_FORWARD_SECURE, QuicPacketNumber(6)); + APPLICATION_DATA, QuicPacketNumber(6)); VerifyLosses(6, packets_acked_, std::vector<uint64_t>{3}); EXPECT_EQ(clock_.Now() + 1.25 * rtt_stats_.smoothed_rtt(), loss_algorithm_.GetLossTimeout()); @@ -140,9 +140,9 @@ AckPackets({4, 5}); unacked_packets_->MaybeUpdateLargestAckedOfPacketNumberSpace( - ENCRYPTION_FORWARD_SECURE, QuicPacketNumber(4)); + APPLICATION_DATA, QuicPacketNumber(4)); unacked_packets_->MaybeUpdateLargestAckedOfPacketNumberSpace( - ENCRYPTION_ZERO_RTT, QuicPacketNumber(5)); + HANDSHAKE_DATA, QuicPacketNumber(5)); // No packet loss by acking 5. VerifyLosses(5, packets_acked_, std::vector<uint64_t>{}); EXPECT_EQ(clock_.Now() + 1.25 * rtt_stats_.smoothed_rtt(),
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 307104d..efc89d3 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -1122,8 +1122,17 @@ QUIC_DLOG(INFO) << ENDPOINT << "Received an old ack frame: ignoring"; return true; } - bool acked_new_packet = - sent_packet_manager_.OnAckFrameEnd(time_of_last_received_packet_); + const AckResult ack_result = sent_packet_manager_.OnAckFrameEnd( + time_of_last_received_packet_, last_decrypted_packet_level_); + if (ack_result != PACKETS_NEWLY_ACKED && + ack_result != NO_PACKETS_NEWLY_ACKED) { + // Error occurred (e.g., this ACK tries to ack packets in wrong packet + // number space), and this would cause the connection to be closed. + QUIC_DLOG(ERROR) << ENDPOINT + << "Error occurred when processing an ACK frame: " + << QuicUtils::AckResultToString(ack_result); + return false; + } // Cancel the send alarm because new packets likely have been acked, which // may change the congestion window and/or pacing rate. Canceling the alarm // causes CanWrite to recalculate the next send time. @@ -1141,7 +1150,8 @@ // If the incoming ack's packets set expresses received packets: peer is still // acking packets which we never care about. // Send an ack to raise the high water mark. - PostProcessAfterAckFrame(GetLeastUnacked() > start, acked_new_packet); + PostProcessAfterAckFrame(GetLeastUnacked() > start, + ack_result == PACKETS_NEWLY_ACKED); processing_ack_frame_ = false; return connected_;
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index f227190..456e81c 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -273,7 +273,7 @@ } } -void QuicSentPacketManager::PostProcessAfterMarkingPacketHandled( +void QuicSentPacketManager::PostProcessNewlyAckedPackets( const QuicAckFrame& ack_frame, QuicTime ack_receive_time, bool rtt_updated, @@ -1139,7 +1139,9 @@ } } -bool QuicSentPacketManager::OnAckFrameEnd(QuicTime ack_receive_time) { +AckResult QuicSentPacketManager::OnAckFrameEnd( + QuicTime ack_receive_time, + EncryptionLevel ack_decrypted_level) { QuicByteCount prior_bytes_in_flight = unacked_packets_.bytes_in_flight(); // Reverse packets_acked_ so that it is in ascending order. reverse(packets_acked_.begin(), packets_acked_.end()); @@ -1154,20 +1156,37 @@ << ", least_unacked: " << unacked_packets_.GetLeastUnacked() << ", packets_acked_: " << packets_acked_; } else { - QUIC_PEER_BUG << "Received ack for unackable packet: " + QUIC_PEER_BUG << "Received " + << QuicUtils::EncryptionLevelToString(ack_decrypted_level) + << " ack for unackable packet: " << acked_packet.packet_number << " with state: " << QuicUtils::SentPacketStateToString(info->state); + if (supports_multiple_packet_number_spaces()) { + if (info->state == NEVER_SENT) { + return UNSENT_PACKETS_ACKED; + } + return UNACKABLE_PACKETS_ACKED; + } } continue; } - QUIC_DVLOG(1) << ENDPOINT << "Got an ack for packet " - << acked_packet.packet_number; + QUIC_DVLOG(1) << ENDPOINT << "Got an " + << QuicUtils::EncryptionLevelToString(ack_decrypted_level) + << " ack for packet " << acked_packet.packet_number; + const PacketNumberSpace packet_number_space = + unacked_packets_.use_uber_loss_algorithm() + ? unacked_packets_.GetPacketNumberSpace(info->encryption_level) + : NUM_PACKET_NUMBER_SPACES; + if (supports_multiple_packet_number_spaces() && + QuicUtils::GetPacketNumberSpace(ack_decrypted_level) != + packet_number_space) { + return PACKETS_ACKED_IN_WRONG_PACKET_NUMBER_SPACE; + } last_ack_frame_.packets.Add(acked_packet.packet_number); largest_packet_peer_knows_is_acked_.UpdateMax(info->largest_acked); if (supports_multiple_packet_number_spaces()) { - largest_packets_peer_knows_is_acked_[QuicUtils::GetPacketNumberSpace( - info->encryption_level)] - .UpdateMax(info->largest_acked); + largest_packets_peer_knows_is_acked_[packet_number_space].UpdateMax( + info->largest_acked); } // If data is associated with the most recent transmission of this // packet, then inform the caller. @@ -1179,16 +1198,16 @@ } if (unacked_packets_.use_uber_loss_algorithm()) { unacked_packets_.MaybeUpdateLargestAckedOfPacketNumberSpace( - info->encryption_level, acked_packet.packet_number); + packet_number_space, acked_packet.packet_number); } MarkPacketHandled(acked_packet.packet_number, info, last_ack_frame_.ack_delay_time); } const bool acked_new_packet = !packets_acked_.empty(); - PostProcessAfterMarkingPacketHandled(last_ack_frame_, ack_receive_time, - rtt_updated_, prior_bytes_in_flight); + PostProcessNewlyAckedPackets(last_ack_frame_, ack_receive_time, rtt_updated_, + prior_bytes_in_flight); - return acked_new_packet; + return acked_new_packet ? PACKETS_NEWLY_ACKED : NO_PACKETS_NEWLY_ACKED; } void QuicSentPacketManager::SetDebugDelegate(DebugDelegate* debug_delegate) {
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index 039ee59..7e10b77 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -271,9 +271,9 @@ // the timestamp field is set. Otherwise, the timestamp is ignored. void OnAckTimestamp(QuicPacketNumber packet_number, QuicTime timestamp); - // Called when an ack frame is parsed completely. Returns true if a previously - // -unacked packet is acked. - bool OnAckFrameEnd(QuicTime ack_receive_time); + // Called when an ack frame is parsed completely. + AckResult OnAckFrameEnd(QuicTime ack_receive_time, + EncryptionLevel ack_decrypted_level); // Called to enable/disable letting session decide what to write. void SetSessionDecideWhatToWrite(bool session_decides_what_to_write) { @@ -491,11 +491,10 @@ QuicTransmissionInfo* transmission_info); // Called after packets have been marked handled with last received ack frame. - void PostProcessAfterMarkingPacketHandled( - const QuicAckFrame& ack_frame, - QuicTime ack_receive_time, - bool rtt_updated, - QuicByteCount prior_bytes_in_flight); + void PostProcessNewlyAckedPackets(const QuicAckFrame& ack_frame, + QuicTime ack_receive_time, + bool rtt_updated, + QuicByteCount prior_bytes_in_flight); // Notify observers that packet with QuicTransmissionInfo |info| is a spurious // retransmission. It is caller's responsibility to guarantee the packet with
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index 4624a8a..08e307c 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -366,7 +366,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(2), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false)); } @@ -397,7 +398,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); // There should no longer be a pending retransmission. EXPECT_FALSE(manager_.HasPendingRetransmissions()); @@ -451,7 +453,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false)); } @@ -467,7 +470,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(2), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(3)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); } EXPECT_EQ(1u, stats_.packets_spuriously_retransmitted); @@ -484,7 +488,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); SendDataPacket(3); SendDataPacket(4); @@ -497,14 +502,16 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(4)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); ExpectAck(4); manager_.OnAckFrameStart(QuicPacketNumber(4), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(5)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); ExpectAckAndLoss(true, 5, 2); if (manager_.session_decides_what_to_write()) { @@ -518,7 +525,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(6)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { uint64_t unacked[] = {2}; @@ -554,7 +562,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); // Since 2 was marked for retransmit, when 1 is acked, 2 is kept for RTT. uint64_t unacked[] = {2}; @@ -591,7 +600,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { // Frames in packets 2 and 3 are acked. EXPECT_CALL(notifier_, IsFrameOutstanding(_)) @@ -619,7 +629,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(5)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); uint64_t unacked2[] = {2}; VerifyUnackedPackets(unacked2, QUIC_ARRAYSIZE(unacked2)); @@ -640,7 +651,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(6)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { uint64_t unacked[] = {2}; @@ -673,7 +685,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); } SendDataPacket(3); @@ -686,7 +699,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(5)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); RetransmitAndSendPacket(3, 5, LOSS_RETRANSMISSION); } @@ -701,7 +715,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(5)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { // Ack 3 will not cause 5 be considered as a spurious retransmission. Ack // 5 will cause 5 be considered as a spurious retransmission as no new @@ -713,7 +728,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(6)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); } } } @@ -738,7 +754,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(2), QuicTime::Delta::FromMilliseconds(5), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(3)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_EQ(QuicPacketNumber(1), manager_.largest_packet_peer_knows_is_acked()); SendAckPacket(3, 3); @@ -749,7 +766,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(3), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(4)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_EQ(QuicPacketNumber(3u), manager_.largest_packet_peer_knows_is_acked()); } @@ -763,7 +781,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_EQ(expected_rtt, manager_.GetRttStats()->latest_rtt()); } @@ -779,7 +798,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::FromMilliseconds(11), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_EQ(expected_rtt, manager_.GetRttStats()->latest_rtt()); } @@ -794,7 +814,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_EQ(expected_rtt, manager_.GetRttStats()->latest_rtt()); } @@ -809,7 +830,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Zero(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_EQ(expected_rtt, manager_.GetRttStats()->latest_rtt()); } @@ -859,7 +881,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(3), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(4)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_TRUE(QuicSentPacketManagerPeer::HasPendingPackets(&manager_)); @@ -880,7 +903,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(5), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(6)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_FALSE(manager_.HasPendingRetransmissions()); EXPECT_FALSE(QuicSentPacketManagerPeer::HasPendingPackets(&manager_)); @@ -988,7 +1012,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(103), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(103), QuicPacketNumber(104)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); // All packets before 103 should be lost. if (manager_.session_decides_what_to_write()) { // Packet 104 is still in flight. @@ -1054,7 +1079,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(8), QuicPacketNumber(10)); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(6)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_FALSE(QuicSentPacketManagerPeer::HasUnackedCryptoPackets(&manager_)); } @@ -1125,7 +1151,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(9), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(8), QuicPacketNumber(10)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { EXPECT_CALL(notifier_, HasUnackedCryptoData()) .WillRepeatedly(Return(false)); @@ -1170,7 +1197,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(2), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_FALSE(QuicSentPacketManagerPeer::HasUnackedCryptoPackets(&manager_)); uint64_t unacked[] = {3}; @@ -1290,7 +1318,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(3), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(4)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); VerifyUnackedPackets(nullptr, 0); VerifyRetransmittablePackets(nullptr, 0); } @@ -1354,7 +1383,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(102), QuicTime::Delta::Zero(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(102), QuicPacketNumber(103)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); } TEST_P(QuicSentPacketManagerTest, RetransmissionTimeoutOnePacket) { @@ -1466,7 +1496,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(102), QuicTime::Delta::Zero(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(102), QuicPacketNumber(103)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); } TEST_P(QuicSentPacketManagerTest, TwoRetransmissionTimeoutsAckSecond) { @@ -1514,7 +1545,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(2), QuicTime::Delta::Zero(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); // The original packet and newest should be outstanding. EXPECT_EQ(2 * kDefaultLength, @@ -1566,7 +1598,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(3), QuicTime::Delta::Zero(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(4)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); // The first two packets should still be outstanding. EXPECT_EQ(2 * kDefaultLength, @@ -1768,7 +1801,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(2), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_FALSE(manager_.HasPendingRetransmissions()); EXPECT_EQ(5 * kDefaultLength, QuicSentPacketManagerPeer::GetBytesInFlight(&manager_)); @@ -1903,7 +1937,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(2), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); QuicTime timeout(clock_.Now() + QuicTime::Delta::FromMilliseconds(10)); EXPECT_CALL(*loss_algorithm, GetLossTimeout()) @@ -2405,7 +2440,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); } TEST_P(QuicSentPacketManagerTest, OnAckRangeSlowPath) { @@ -2426,7 +2462,8 @@ manager_.OnAckRange(QuicPacketNumber(5), QuicPacketNumber(7)); // Make sure empty range does not harm. manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(4)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); // Ack [4, 8), [9, 13), [14, 21). uint64_t acked2[] = {4, 7, 9, 12, 14, 17, 18, 19, 20}; @@ -2436,7 +2473,8 @@ manager_.OnAckRange(QuicPacketNumber(14), QuicPacketNumber(21)); manager_.OnAckRange(QuicPacketNumber(9), QuicPacketNumber(13)); manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(8)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); } TEST_P(QuicSentPacketManagerTest, TolerateReneging) { @@ -2458,7 +2496,8 @@ manager_.OnAckRange(QuicPacketNumber(15), QuicPacketNumber(17)); manager_.OnAckRange(QuicPacketNumber(10), QuicPacketNumber(12)); manager_.OnAckRange(QuicPacketNumber(5), QuicPacketNumber(7)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); // Making sure reneged ACK does not harm. Ack [4, 8), [9, 13). uint64_t acked2[] = {4, 7, 9, 12}; @@ -2467,7 +2506,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(9), QuicPacketNumber(13)); manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(8)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_EQ(QuicPacketNumber(16), manager_.GetLargestObserved()); } @@ -2491,7 +2531,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_EQ(QuicPacketNumber(1), manager_.GetLargestAckedPacket(ENCRYPTION_INITIAL)); EXPECT_FALSE( @@ -2510,7 +2551,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(2), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_HANDSHAKE)); EXPECT_EQ(QuicPacketNumber(2), manager_.GetLargestAckedPacket(ENCRYPTION_HANDSHAKE)); EXPECT_FALSE( @@ -2520,7 +2562,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(3), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(4)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_HANDSHAKE)); EXPECT_EQ(QuicPacketNumber(3), manager_.GetLargestAckedPacket(ENCRYPTION_HANDSHAKE)); EXPECT_FALSE( @@ -2541,7 +2584,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(5), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(5), QuicPacketNumber(6)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_FORWARD_SECURE)); EXPECT_EQ(QuicPacketNumber(3), manager_.GetLargestAckedPacket(ENCRYPTION_HANDSHAKE)); EXPECT_EQ(QuicPacketNumber(5), @@ -2567,7 +2611,8 @@ manager_.OnAckFrameStart(QuicPacketNumber(8), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(9)); - EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now())); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_FORWARD_SECURE)); EXPECT_EQ(QuicPacketNumber(3), manager_.GetLargestAckedPacket(ENCRYPTION_HANDSHAKE)); EXPECT_EQ(QuicPacketNumber(8), @@ -2576,6 +2621,75 @@ manager_.GetLargestAckedPacket(ENCRYPTION_FORWARD_SECURE)); } +TEST_P(QuicSentPacketManagerTest, PacketsGetAckedInWrongPacketNumberSpace) { + if (!GetQuicReloadableFlag(quic_use_uber_loss_algorithm)) { + return; + } + manager_.EnableMultiplePacketNumberSpacesSupport(); + // Send packet 1. + SendDataPacket(1, ENCRYPTION_INITIAL); + // Send packets 2 and 3. + SendDataPacket(2, ENCRYPTION_HANDSHAKE); + SendDataPacket(3, ENCRYPTION_HANDSHAKE); + + // ACK packets 2 and 3 in the wrong packet number space. + manager_.OnAckFrameStart(QuicPacketNumber(3), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(4)); + EXPECT_EQ(PACKETS_ACKED_IN_WRONG_PACKET_NUMBER_SPACE, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); +} + +TEST_P(QuicSentPacketManagerTest, PacketsGetAckedInWrongPacketNumberSpace2) { + if (!GetQuicReloadableFlag(quic_use_uber_loss_algorithm)) { + return; + } + manager_.EnableMultiplePacketNumberSpacesSupport(); + // Send packet 1. + SendDataPacket(1, ENCRYPTION_INITIAL); + // Send packets 2 and 3. + SendDataPacket(2, ENCRYPTION_HANDSHAKE); + SendDataPacket(3, ENCRYPTION_HANDSHAKE); + + // ACK packet 1 in the wrong packet number space. + manager_.OnAckFrameStart(QuicPacketNumber(3), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(4)); + EXPECT_EQ(PACKETS_ACKED_IN_WRONG_PACKET_NUMBER_SPACE, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_HANDSHAKE)); +} + +TEST_P(QuicSentPacketManagerTest, + ToleratePacketsGetAckedInWrongPacketNumberSpace) { + if (!GetQuicReloadableFlag(quic_use_uber_loss_algorithm)) { + return; + } + manager_.EnableMultiplePacketNumberSpacesSupport(); + // Send packet 1. + SendDataPacket(1, ENCRYPTION_INITIAL); + // Ack packet 1. + ExpectAck(1); + manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + + // Send packets 2 and 3. + SendDataPacket(2, ENCRYPTION_HANDSHAKE); + SendDataPacket(3, ENCRYPTION_HANDSHAKE); + + // Packet 1 gets acked in the wrong packet number space. Since packet 1 has + // been acked in the correct packet number space, tolerate it. + uint64_t acked[] = {2, 3}; + ExpectAcksAndLosses(true, acked, QUIC_ARRAYSIZE(acked), nullptr, 0); + manager_.OnAckFrameStart(QuicPacketNumber(3), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(4)); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_HANDSHAKE)); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_types.h b/quic/core/quic_types.h index 19dac06..f90b0fa 100644 --- a/quic/core/quic_types.h +++ b/quic/core/quic_types.h
@@ -587,6 +587,20 @@ enum AckMode { TCP_ACKING, ACK_DECIMATION, ACK_DECIMATION_WITH_REORDERING }; +// Used to return the result of processing a received ACK frame. +enum AckResult { + PACKETS_NEWLY_ACKED, + NO_PACKETS_NEWLY_ACKED, + UNSENT_PACKETS_ACKED, // Peer acks unsent packets. + UNACKABLE_PACKETS_ACKED, // Peer acks packets that are not expected to be + // acked. For example, encryption is reestablished, + // and all sent encrypted packets cannot be + // decrypted by the peer. Version gets negotiated, + // and all sent packets in the different version + // cannot be processed by the peer. + PACKETS_ACKED_IN_WRONG_PACKET_NUMBER_SPACE, +}; + } // namespace quic #endif // QUICHE_QUIC_CORE_QUIC_TYPES_H_
diff --git a/quic/core/quic_unacked_packet_map.cc b/quic/core/quic_unacked_packet_map.cc index 5a9bd24..1a0b1af 100644 --- a/quic/core/quic_unacked_packet_map.cc +++ b/quic/core/quic_unacked_packet_map.cc
@@ -229,11 +229,9 @@ } void QuicUnackedPacketMap::MaybeUpdateLargestAckedOfPacketNumberSpace( - EncryptionLevel encryption_level, + PacketNumberSpace packet_number_space, QuicPacketNumber packet_number) { DCHECK(use_uber_loss_algorithm_); - const PacketNumberSpace packet_number_space = - GetPacketNumberSpace(encryption_level); largest_acked_packets_[packet_number_space].UpdateMax(packet_number); }
diff --git a/quic/core/quic_unacked_packet_map.h b/quic/core/quic_unacked_packet_map.h index 1d4f00c..1a37a1f 100644 --- a/quic/core/quic_unacked_packet_map.h +++ b/quic/core/quic_unacked_packet_map.h
@@ -166,9 +166,9 @@ void IncreaseLargestAcked(QuicPacketNumber largest_acked); // Called when |packet_number| gets acked. Maybe increase the largest acked of - // corresponding packet number space of |encryption_level|. + // |packet_number_space|. void MaybeUpdateLargestAckedOfPacketNumberSpace( - EncryptionLevel encryption_level, + PacketNumberSpace packet_number_space, QuicPacketNumber packet_number); // Remove any packets no longer needed for retransmission, congestion, or
diff --git a/quic/core/quic_utils.cc b/quic/core/quic_utils.cc index 56cfe87..413a244 100644 --- a/quic/core/quic_utils.cc +++ b/quic/core/quic_utils.cc
@@ -209,6 +209,18 @@ } // static +const char* QuicUtils::AckResultToString(AckResult result) { + switch (result) { + RETURN_STRING_LITERAL(PACKETS_NEWLY_ACKED); + RETURN_STRING_LITERAL(NO_PACKETS_NEWLY_ACKED); + RETURN_STRING_LITERAL(UNSENT_PACKETS_ACKED); + RETURN_STRING_LITERAL(UNACKABLE_PACKETS_ACKED); + RETURN_STRING_LITERAL(PACKETS_ACKED_IN_WRONG_PACKET_NUMBER_SPACE); + } + return "INVALID_ACK_RESULT"; +} + +// static AddressChangeType QuicUtils::DetermineAddressChangeType( const QuicSocketAddress& old_address, const QuicSocketAddress& new_address) {
diff --git a/quic/core/quic_utils.h b/quic/core/quic_utils.h index 340f474..cacbc60 100644 --- a/quic/core/quic_utils.h +++ b/quic/core/quic_utils.h
@@ -64,6 +64,9 @@ // Returns QuicLongHeaderType as a char*. static const char* QuicLongHeaderTypetoString(QuicLongHeaderType type); + // Returns AckResult as a char*. + static const char* AckResultToString(AckResult result); + // Determines and returns change type of address change from |old_address| to // |new_address|. static AddressChangeType DetermineAddressChangeType(