Let QuicConnection::UpdatePacketContent() return connection state. This allows callers to correctly handle errors occurred. Protected by quic_reloadable_flag_quic_update_packet_content_returns_connected. PiperOrigin-RevId: 353360764 Change-Id: Ie1e9874398311cf3f3f6873bac5dc53d6b1a94d8
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 5090991..bf8cd2d 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -1262,7 +1262,9 @@ // Since a stream frame was received, this is not a connectivity probe. // A probe only contains a PING and full padding. - UpdatePacketContent(STREAM_FRAME); + if (!UpdatePacketContent(STREAM_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnStreamFrame(frame); @@ -1300,7 +1302,9 @@ // Since a CRYPTO frame was received, this is not a connectivity probe. // A probe only contains a PING and full padding. - UpdatePacketContent(CRYPTO_FRAME); + if (!UpdatePacketContent(CRYPTO_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnCryptoFrame(frame); @@ -1325,7 +1329,9 @@ // Since an ack frame was received, this is not a connectivity probe. // A probe only contains a PING and full padding. - UpdatePacketContent(ACK_FRAME); + if (!UpdatePacketContent(ACK_FRAME)) { + return false; + } QUIC_DVLOG(1) << ENDPOINT << "OnAckFrameStart, largest_acked: " << largest_acked; @@ -1459,7 +1465,9 @@ // Since a stop waiting frame was received, this is not a connectivity probe. // A probe only contains a PING and full padding. - UpdatePacketContent(STOP_WAITING_FRAME); + if (!UpdatePacketContent(STOP_WAITING_FRAME)) { + return false; + } if (no_stop_waiting_frames_) { return true; @@ -1492,7 +1500,9 @@ QUIC_BUG_IF(!connected_) << "Processing PADDING frame when connection is closed. Last frame: " << most_recent_frame_type_; - UpdatePacketContent(PADDING_FRAME); + if (!UpdatePacketContent(PADDING_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnPaddingFrame(frame); @@ -1504,7 +1514,9 @@ QUIC_BUG_IF(!connected_) << "Processing PING frame when connection is closed. Last frame: " << most_recent_frame_type_; - UpdatePacketContent(PING_FRAME); + if (!UpdatePacketContent(PING_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { QuicTime::Delta ping_received_delay = QuicTime::Delta::Zero(); @@ -1549,7 +1561,9 @@ // Since a reset stream frame was received, this is not a connectivity probe. // A probe only contains a PING and full padding. - UpdatePacketContent(RST_STREAM_FRAME); + if (!UpdatePacketContent(RST_STREAM_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnRstStreamFrame(frame); @@ -1570,7 +1584,9 @@ // Since a reset stream frame was received, this is not a connectivity probe. // A probe only contains a PING and full padding. - UpdatePacketContent(STOP_SENDING_FRAME); + if (!UpdatePacketContent(STOP_SENDING_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnStopSendingFrame(frame); @@ -1594,7 +1610,9 @@ // Only respond to the 1st PATH_CHALLENGE. return true; } - UpdatePacketContent(PATH_CHALLENGE_FRAME); + if (!UpdatePacketContent(PATH_CHALLENGE_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnPathChallengeFrame(frame); } @@ -1630,7 +1648,9 @@ QUIC_BUG_IF(!connected_) << "Processing PATH_RESPONSE frame when connection " "is closed. Last frame: " << most_recent_frame_type_; - UpdatePacketContent(PATH_RESPONSE_FRAME); + if (!UpdatePacketContent(PATH_RESPONSE_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnPathResponseFrame(frame); } @@ -1658,7 +1678,9 @@ // Since a connection close frame was received, this is not a connectivity // probe. A probe only contains a PING and full padding. - UpdatePacketContent(CONNECTION_CLOSE_FRAME); + if (!UpdatePacketContent(CONNECTION_CLOSE_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnConnectionCloseFrame(frame); @@ -1703,7 +1725,10 @@ QUIC_BUG_IF(!connected_) << "Processing MAX_STREAMS frame when connection is closed. Last frame: " << most_recent_frame_type_; - UpdatePacketContent(MAX_STREAMS_FRAME); + if (!UpdatePacketContent(MAX_STREAMS_FRAME)) { + return false; + } + if (debug_visitor_ != nullptr) { debug_visitor_->OnMaxStreamsFrame(frame); } @@ -1715,7 +1740,10 @@ QUIC_BUG_IF(!connected_) << "Processing STREAMS_BLOCKED frame when " "connection is closed. Last frame: " << most_recent_frame_type_; - UpdatePacketContent(STREAMS_BLOCKED_FRAME); + if (!UpdatePacketContent(STREAMS_BLOCKED_FRAME)) { + return false; + } + if (debug_visitor_ != nullptr) { debug_visitor_->OnStreamsBlockedFrame(frame); } @@ -1729,7 +1757,9 @@ // Since a go away frame was received, this is not a connectivity probe. // A probe only contains a PING and full padding. - UpdatePacketContent(GOAWAY_FRAME); + if (!UpdatePacketContent(GOAWAY_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnGoAwayFrame(frame); @@ -1750,7 +1780,9 @@ // Since a window update frame was received, this is not a connectivity probe. // A probe only contains a PING and full padding. - UpdatePacketContent(WINDOW_UPDATE_FRAME); + if (!UpdatePacketContent(WINDOW_UPDATE_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnWindowUpdateFrame( @@ -1767,7 +1799,10 @@ QUIC_BUG_IF(!connected_) << "Processing NEW_CONNECTION_ID frame when " "connection is closed. Last frame: " << most_recent_frame_type_; - UpdatePacketContent(NEW_CONNECTION_ID_FRAME); + if (!UpdatePacketContent(NEW_CONNECTION_ID_FRAME)) { + return false; + } + if (debug_visitor_ != nullptr) { debug_visitor_->OnNewConnectionIdFrame(frame); } @@ -1779,7 +1814,10 @@ QUIC_BUG_IF(!connected_) << "Processing RETIRE_CONNECTION_ID frame when " "connection is closed. Last frame: " << most_recent_frame_type_; - UpdatePacketContent(RETIRE_CONNECTION_ID_FRAME); + if (!UpdatePacketContent(RETIRE_CONNECTION_ID_FRAME)) { + return false; + } + if (debug_visitor_ != nullptr) { debug_visitor_->OnRetireConnectionIdFrame(frame); } @@ -1790,7 +1828,10 @@ QUIC_BUG_IF(!connected_) << "Processing NEW_TOKEN frame when connection is closed. Last frame: " << most_recent_frame_type_; - UpdatePacketContent(NEW_TOKEN_FRAME); + if (!UpdatePacketContent(NEW_TOKEN_FRAME)) { + return false; + } + if (debug_visitor_ != nullptr) { debug_visitor_->OnNewTokenFrame(frame); } @@ -1815,7 +1856,9 @@ // Since a message frame was received, this is not a connectivity probe. // A probe only contains a PING and full padding. - UpdatePacketContent(MESSAGE_FRAME); + if (!UpdatePacketContent(MESSAGE_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnMessageFrame(frame); @@ -1846,7 +1889,9 @@ // Since a handshake done frame was received, this is not a connectivity // probe. A probe only contains a PING and full padding. - UpdatePacketContent(HANDSHAKE_DONE_FRAME); + if (!UpdatePacketContent(HANDSHAKE_DONE_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnHandshakeDoneFrame(frame); @@ -1863,7 +1908,10 @@ if (debug_visitor_ != nullptr) { debug_visitor_->OnAckFrequencyFrame(frame); } - UpdatePacketContent(ACK_FREQUENCY_FRAME); + if (!UpdatePacketContent(ACK_FREQUENCY_FRAME)) { + return false; + } + if (!can_receive_ack_frequency_frame_) { QUIC_LOG_EVERY_N_SEC(ERROR, 120) << "Get unexpected AckFrequencyFrame."; return false; @@ -1888,7 +1936,9 @@ // Since a blocked frame was received, this is not a connectivity probe. // A probe only contains a PING and full padding. - UpdatePacketContent(BLOCKED_FRAME); + if (!UpdatePacketContent(BLOCKED_FRAME)) { + return false; + } if (debug_visitor_ != nullptr) { debug_visitor_->OnBlockedFrame(frame); @@ -4777,19 +4827,22 @@ sent_packet_manager_.OnApplicationLimited(); } -void QuicConnection::UpdatePacketContent(QuicFrameType type) { +bool QuicConnection::UpdatePacketContent(QuicFrameType type) { + if (update_packet_content_returns_connected_) { + QUIC_RELOADABLE_FLAG_COUNT(quic_update_packet_content_returns_connected); + } most_recent_frame_type_ = type; if (version().HasIetfQuicFrames()) { if (!QuicUtils::IsProbingFrame(type)) { MaybeStartIetfPeerMigration(); - return; + return !update_packet_content_returns_connected_ || connected_; } QuicSocketAddress current_effective_peer_address = GetEffectivePeerAddressFromCurrentPacket(); if (!count_bytes_on_alternative_path_seperately_ || IsDefaultPath(last_packet_destination_address_, last_packet_source_address_)) { - return; + return !update_packet_content_returns_connected_ || connected_; } QUIC_CODE_COUNT_N(quic_count_bytes_on_alternative_path_seperately, 3, 5); if (type == PATH_CHALLENGE_FRAME && @@ -4804,7 +4857,7 @@ last_packet_destination_address_, current_effective_peer_address); } MaybeUpdateBytesReceivedFromAlternativeAddress(last_size_); - return; + return !update_packet_content_returns_connected_ || connected_; } // Packet content is tracked to identify connectivity probe in non-IETF // version, where a connectivity probe is defined as @@ -4815,13 +4868,13 @@ // We have already learned the current packet is not a connectivity // probing packet. Peer migration should have already been started earlier // if needed. - return; + return !update_packet_content_returns_connected_ || connected_; } if (type == PING_FRAME) { if (current_packet_content_ == NO_FRAMES_RECEIVED) { current_packet_content_ = FIRST_FRAME_IS_PING; - return; + return !update_packet_content_returns_connected_ || connected_; } } @@ -4851,7 +4904,7 @@ << last_packet_destination_address_ << ", self_address_:" << self_address_; } - return; + return !update_packet_content_returns_connected_ || connected_; } current_packet_content_ = NOT_PADDED_PING; @@ -4865,6 +4918,7 @@ } } current_effective_peer_migration_type_ = NO_CHANGE; + return !update_packet_content_returns_connected_ || connected_; } void QuicConnection::MaybeStartIetfPeerMigration() {
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index 45211d2..b250810 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -54,6 +54,7 @@ #include "quic/core/uber_received_packet_manager.h" #include "quic/platform/api/quic_containers.h" #include "quic/platform/api/quic_export.h" +#include "quic/platform/api/quic_flags.h" #include "quic/platform/api/quic_socket_address.h" #include "common/platform/api/quiche_str_cat.h" @@ -1450,7 +1451,8 @@ // starts effective peer migration if current packet is confirmed not a // connectivity probe and |current_effective_peer_migration_type_| indicates // effective peer address change. - void UpdatePacketContent(QuicFrameType type); + // Returns true if connection is still alive. + ABSL_MUST_USE_RESULT bool UpdatePacketContent(QuicFrameType type); // Called when last received ack frame has been processed. // |send_stop_waiting| indicates whether a stop waiting needs to be sent. @@ -2049,6 +2051,9 @@ bool count_bytes_on_alternative_path_seperately_ = GetQuicReloadableFlag(quic_count_bytes_on_alternative_path_seperately); + + bool update_packet_content_returns_connected_ = + GetQuicReloadableFlag(quic_update_packet_content_returns_connected); }; } // namespace quic
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 29ad505..5643114 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -13265,6 +13265,7 @@ QUIC_INVALID_0RTT_PACKET_NUMBER_OUT_OF_ORDER); } +// Regression test for b/177312785 TEST_P(QuicConnectionTest, PeerMigrateBeforeHandshakeConfirm) { if (!VersionHasIetfQuicFrames(version().transport_version) || !GetQuicReloadableFlag(quic_start_peer_migration_earlier)) { @@ -13294,12 +13295,22 @@ // Process another packet with a different peer address on server side will // close connection. + QuicAckFrame frame = InitAckFrame(1); EXPECT_CALL(visitor_, BeforeConnectionCloseSent()); EXPECT_CALL(visitor_, OnConnectionClosed(_, ConnectionCloseSource::FROM_SELF)); EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0u); - ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, - kNewPeerAddress, ENCRYPTION_INITIAL); + if (!GetQuicReloadableFlag(quic_update_packet_content_returns_connected)) { + EXPECT_CALL(*send_algorithm_, OnCongestionEvent(_, _, _, _, _)); + EXPECT_QUIC_BUG( + ProcessFramePacketWithAddresses(QuicFrame(&frame), kSelfAddress, + kNewPeerAddress, ENCRYPTION_INITIAL), + ""); + } else { + EXPECT_CALL(*send_algorithm_, OnCongestionEvent(_, _, _, _, _)).Times(0); + ProcessFramePacketWithAddresses(QuicFrame(&frame), kSelfAddress, + kNewPeerAddress, ENCRYPTION_INITIAL); + } EXPECT_FALSE(connection_.connected()); }
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index aa3b43d..73ae73e 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -67,6 +67,7 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_tls_use_early_select_cert, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_tls_use_per_handshaker_proof_source, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_unified_iw_options, false) +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_update_packet_content_returns_connected, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_use_circular_deque_for_unacked_packets_v2, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_use_encryption_level_context, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_use_write_or_buffer_data_at_level, false)