Modifications to QuicPathValidator. Add effective peer address to PathValidationContext and pass it to SendPathChallenge(). CancelPathValidation() to call validation failure callback. Add a QUIC_BUG to prevent StartValidatingPath() to be called while there is on-going validation. PiperOrigin-RevId: 348542080 Change-Id: I5ce4f2a7ea4b621ce6a2c25c1852a86b454c9b58
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 1599d10..a8067cd 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -5380,10 +5380,12 @@ return sent_packet_manager_.GetRetransmissionTime(); } -bool QuicConnection::SendPathChallenge(const QuicPathFrameBuffer& data_buffer, - const QuicSocketAddress& self_address, - const QuicSocketAddress& peer_address, - QuicPacketWriter* writer) { +bool QuicConnection::SendPathChallenge( + const QuicPathFrameBuffer& data_buffer, + const QuicSocketAddress& self_address, + const QuicSocketAddress& peer_address, + const QuicSocketAddress& /*effective_peer_address*/, + QuicPacketWriter* writer) { if (writer == writer_) { { // It's on current path, add the PATH_CHALLENGE the same way as other
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index dd453cb..02f70de 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -1129,6 +1129,7 @@ bool SendPathChallenge(const QuicPathFrameBuffer& data_buffer, const QuicSocketAddress& self_address, const QuicSocketAddress& peer_address, + const QuicSocketAddress& effective_peer_address, QuicPacketWriter* writer) override; // If |writer| is the default writer and |peer_address| is the same as // peer_address(), return the PTO of this connection. Otherwise, return 3 *
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 5d26ab0..c1ce8b4 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -6395,8 +6395,9 @@ QuicPathFrameBuffer payload{ {0xde, 0xad, 0xbe, 0xef, 0xba, 0xdc, 0x0f, 0xfe}}; QuicConnection::ScopedPacketFlusher flusher(&connection_); - connection_.SendPathChallenge(payload, connection_.self_address(), - connection_.peer_address(), writer_.get()); + connection_.SendPathChallenge( + payload, connection_.self_address(), connection_.peer_address(), + connection_.effective_peer_address(), writer_.get()); } else { connection_.SendConnectivityProbingPacket(writer_.get(), connection_.peer_address());
diff --git a/quic/core/quic_path_validator.cc b/quic/core/quic_path_validator.cc index fe9e046..0722216 100644 --- a/quic/core/quic_path_validator.cc +++ b/quic/core/quic_path_validator.cc
@@ -59,16 +59,22 @@ probing_data_.end()) { result_delegate_->OnPathValidationSuccess(std::move(path_context_)); ResetPathValidation(); + } else { + QUIC_DVLOG(1) << "PATH_RESPONSE with payload " << probing_data.data() + << " doesn't match the probing data."; } } void QuicPathValidator::StartPathValidation( std::unique_ptr<QuicPathValidationContext> context, std::unique_ptr<ResultDelegate> result_delegate) { - CancelPathValidation(); DCHECK_NE(nullptr, context); QUIC_DLOG(INFO) << "Start validating path " << *context << " via writer: " << context->WriterToUse(); + if (path_context_ != nullptr) { + QUIC_BUG << "There is an on-going validation on path " << *path_context_; + ResetPathValidation(); + } path_context_ = std::move(context); result_delegate_ = std::move(result_delegate); @@ -87,6 +93,7 @@ return; } QUIC_DVLOG(1) << "Cancel validation on path" << *path_context_; + result_delegate_->OnPathValidationFailure(std::move(path_context_)); ResetPathValidation(); } @@ -107,7 +114,6 @@ void QuicPathValidator::OnRetryTimeout() { ++retry_count_; if (retry_count_ > kMaxRetryTimes) { - result_delegate_->OnPathValidationFailure(std::move(path_context_)); CancelPathValidation(); return; } @@ -118,7 +124,8 @@ void QuicPathValidator::SendPathChallengeAndSetAlarm() { bool should_continue = send_delegate_->SendPathChallenge( GeneratePathChallengePayload(), path_context_->self_address(), - path_context_->peer_address(), path_context_->WriterToUse()); + path_context_->peer_address(), path_context_->effective_peer_address(), + path_context_->WriterToUse()); if (!should_continue) { // The delegate doesn't want to continue the path validation.
diff --git a/quic/core/quic_path_validator.h b/quic/core/quic_path_validator.h index b9c9c5d..1463720 100644 --- a/quic/core/quic_path_validator.h +++ b/quic/core/quic_path_validator.h
@@ -31,7 +31,16 @@ public: QuicPathValidationContext(const QuicSocketAddress& self_address, const QuicSocketAddress& peer_address) - : self_address_(self_address), peer_address_(peer_address) {} + : self_address_(self_address), + peer_address_(peer_address), + effective_peer_address_(peer_address) {} + + QuicPathValidationContext(const QuicSocketAddress& self_address, + const QuicSocketAddress& peer_address, + const QuicSocketAddress& effective_peer_address) + : self_address_(self_address), + peer_address_(peer_address), + effective_peer_address_(effective_peer_address) {} virtual ~QuicPathValidationContext() = default; @@ -39,6 +48,9 @@ const QuicSocketAddress& self_address() const { return self_address_; } const QuicSocketAddress& peer_address() const { return peer_address_; } + const QuicSocketAddress& effective_peer_address() const { + return effective_peer_address_; + } private: QUIC_EXPORT_PRIVATE friend std::ostream& operator<<( @@ -46,7 +58,11 @@ const QuicPathValidationContext& context); QuicSocketAddress self_address_; + // The address to send PATH_CHALLENGE. QuicSocketAddress peer_address_; + // The actual peer address which is different from |peer_address_| if the peer + // is behind a proxy. + QuicSocketAddress effective_peer_address_; }; // Used to validate a path by sending up to 3 PATH_CHALLENGE frames before @@ -64,10 +80,12 @@ // Send a PATH_CHALLENGE with |data_buffer| as the frame payload using given // path information. Return false if the delegate doesn't want to continue // the validation. - virtual bool SendPathChallenge(const QuicPathFrameBuffer& data_buffer, - const QuicSocketAddress& self_address, - const QuicSocketAddress& peer_address, - QuicPacketWriter* writer) = 0; + virtual bool SendPathChallenge( + const QuicPathFrameBuffer& data_buffer, + const QuicSocketAddress& self_address, + const QuicSocketAddress& peer_address, + const QuicSocketAddress& effective_peer_address, + QuicPacketWriter* writer) = 0; // Return the time to retry sending PATH_CHALLENGE again based on given peer // address and writer. virtual QuicTime GetRetryTimeout(const QuicSocketAddress& peer_address,
diff --git a/quic/core/quic_path_validator_test.cc b/quic/core/quic_path_validator_test.cc index 5fd7f9f..fab0c7c 100644 --- a/quic/core/quic_path_validator_test.cc +++ b/quic/core/quic_path_validator_test.cc
@@ -35,6 +35,7 @@ (const QuicPathFrameBuffer&, const QuicSocketAddress&, const QuicSocketAddress&, + const QuicSocketAddress&, QuicPacketWriter*), (override)); @@ -50,8 +51,10 @@ : path_validator_(&alarm_factory_, &arena_, &send_delegate_, &random_), context_(new MockQuicPathValidationContext(self_address_, peer_address_, + effective_peer_address_, &writer_)), - result_delegate_(new MockQuicPathValidationResultDelegate()) { + result_delegate_( + new testing::StrictMock<MockQuicPathValidationResultDelegate>()) { clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(1)); ON_CALL(send_delegate_, GetRetryTimeout(_, _)) .WillByDefault( @@ -68,6 +71,7 @@ QuicPathValidator path_validator_; QuicSocketAddress self_address_{QuicIpAddress::Any4(), 443}; QuicSocketAddress peer_address_{QuicIpAddress::Loopback4(), 443}; + QuicSocketAddress effective_peer_address_{QuicIpAddress::Loopback4(), 12345}; MockPacketWriter writer_; MockQuicPathValidationContext* context_; MockQuicPathValidationResultDelegate* result_delegate_; @@ -76,10 +80,11 @@ TEST_F(QuicPathValidatorTest, PathValidationSuccessOnFirstRound) { QuicPathFrameBuffer challenge_data; EXPECT_CALL(send_delegate_, - SendPathChallenge(_, self_address_, peer_address_, &writer_)) + SendPathChallenge(_, self_address_, peer_address_, + effective_peer_address_, &writer_)) .WillOnce(Invoke([&](const QuicPathFrameBuffer& payload, const QuicSocketAddress&, const QuicSocketAddress&, - QuicPacketWriter*) { + const QuicSocketAddress&, QuicPacketWriter*) { memcpy(challenge_data.data(), payload.data(), payload.size()); return true; })); @@ -99,10 +104,11 @@ TEST_F(QuicPathValidatorTest, RespondWithDifferentSelfAddress) { QuicPathFrameBuffer challenge_data; EXPECT_CALL(send_delegate_, - SendPathChallenge(_, self_address_, peer_address_, &writer_)) + SendPathChallenge(_, self_address_, peer_address_, + effective_peer_address_, &writer_)) .WillOnce(Invoke([&](const QuicPathFrameBuffer payload, const QuicSocketAddress&, const QuicSocketAddress&, - QuicPacketWriter*) { + const QuicSocketAddress&, QuicPacketWriter*) { memcpy(challenge_data.data(), payload.data(), payload.size()); return true; })); @@ -126,17 +132,18 @@ TEST_F(QuicPathValidatorTest, RespondAfter1stRetry) { QuicPathFrameBuffer challenge_data; EXPECT_CALL(send_delegate_, - SendPathChallenge(_, self_address_, peer_address_, &writer_)) + SendPathChallenge(_, self_address_, peer_address_, + effective_peer_address_, &writer_)) .WillOnce(Invoke([&](const QuicPathFrameBuffer& payload, const QuicSocketAddress&, const QuicSocketAddress&, - QuicPacketWriter*) { + const QuicSocketAddress&, QuicPacketWriter*) { // Store up the 1st PATH_CHALLANGE payload. memcpy(challenge_data.data(), payload.data(), payload.size()); return true; })) .WillOnce(Invoke([&](const QuicPathFrameBuffer& payload, const QuicSocketAddress&, const QuicSocketAddress&, - QuicPacketWriter*) { + const QuicSocketAddress&, QuicPacketWriter*) { EXPECT_NE(payload, challenge_data); return true; })); @@ -160,16 +167,17 @@ TEST_F(QuicPathValidatorTest, RespondToRetryChallenge) { QuicPathFrameBuffer challenge_data; EXPECT_CALL(send_delegate_, - SendPathChallenge(_, self_address_, peer_address_, &writer_)) + SendPathChallenge(_, self_address_, peer_address_, + effective_peer_address_, &writer_)) .WillOnce(Invoke([&](const QuicPathFrameBuffer& payload, const QuicSocketAddress&, const QuicSocketAddress&, - QuicPacketWriter*) { + const QuicSocketAddress&, QuicPacketWriter*) { memcpy(challenge_data.data(), payload.data(), payload.size()); return true; })) .WillOnce(Invoke([&](const QuicPathFrameBuffer& payload, const QuicSocketAddress&, const QuicSocketAddress&, - QuicPacketWriter*) { + const QuicSocketAddress&, QuicPacketWriter*) { EXPECT_NE(challenge_data, payload); memcpy(challenge_data.data(), payload.data(), payload.size()); return true; @@ -193,7 +201,8 @@ TEST_F(QuicPathValidatorTest, ValidationTimeOut) { EXPECT_CALL(send_delegate_, - SendPathChallenge(_, self_address_, peer_address_, &writer_)) + SendPathChallenge(_, self_address_, peer_address_, + effective_peer_address_, &writer_)) .Times(3u) .WillRepeatedly(Return(true)); EXPECT_CALL(send_delegate_, GetRetryTimeout(peer_address_, &writer_)) @@ -221,9 +230,11 @@ TEST_F(QuicPathValidatorTest, SendPathChallengeError) { EXPECT_CALL(send_delegate_, - SendPathChallenge(_, self_address_, peer_address_, &writer_)) + SendPathChallenge(_, self_address_, peer_address_, + effective_peer_address_, &writer_)) .WillOnce(Invoke([&](const QuicPathFrameBuffer&, const QuicSocketAddress&, - const QuicSocketAddress&, QuicPacketWriter*) { + const QuicSocketAddress&, const QuicSocketAddress&, + QuicPacketWriter*) { // Abandon this validation in the call stack shouldn't cause crash and // should cancel the alarm. path_validator_.CancelPathValidation(); @@ -231,6 +242,7 @@ })); EXPECT_CALL(send_delegate_, GetRetryTimeout(peer_address_, &writer_)) .Times(0u); + EXPECT_CALL(*result_delegate_, OnPathValidationFailure(_)); path_validator_.StartPathValidation( std::unique_ptr<QuicPathValidationContext>(context_), std::unique_ptr<MockQuicPathValidationResultDelegate>(result_delegate_));
diff --git a/quic/test_tools/quic_test_utils.h b/quic/test_tools/quic_test_utils.h index 428d695..e78839a 100644 --- a/quic/test_tools/quic_test_utils.h +++ b/quic/test_tools/quic_test_utils.h
@@ -734,6 +734,7 @@ (const QuicPathFrameBuffer&, const QuicSocketAddress&, const QuicSocketAddress&, + const QuicSocketAddress&, QuicPacketWriter*), (override)); @@ -1658,8 +1659,11 @@ public: MockQuicPathValidationContext(const QuicSocketAddress& self_address, const QuicSocketAddress& peer_address, + const QuicSocketAddress& effective_peer_address, QuicPacketWriter* writer) - : QuicPathValidationContext(self_address, peer_address), + : QuicPathValidationContext(self_address, + peer_address, + effective_peer_address), writer_(writer) {} QuicPacketWriter* WriterToUse() override { return writer_; }