From f2431a97d57a48510db9827a672d8835ef3fa537 Mon Sep 17 00:00:00 2001 From: Tommi Date: Tue, 20 Feb 2024 15:27:44 +0100 Subject: [PATCH] Move ComputeFoundation to Candidate. Move code from P2PTransportChannel to Candidate, where we set the foundation value for remote prflx candidates. Bug: none Change-Id: I7dbcb85bca35dca7297136b0706092dd8d2b153c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339902 Reviewed-by: Philipp Hancke Commit-Queue: Tomas Gunnarsson Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#41774} --- api/BUILD.gn | 2 ++ api/candidate.cc | 41 ++++++++++++++++++++++++- api/candidate.h | 35 ++++++++++++++++++++++ api/candidate_unittest.cc | 48 ++++++++++++++++++++++++++++-- p2p/BUILD.gn | 2 -- p2p/base/connection.cc | 5 ++-- p2p/base/p2p_transport_channel.cc | 4 +-- p2p/base/port.cc | 33 ++++++++------------ p2p/base/port.h | 12 -------- p2p/base/port_interface.h | 12 -------- pc/rtc_stats_collector_unittest.cc | 24 +++++++++------ 11 files changed, 153 insertions(+), 65 deletions(-) diff --git a/api/BUILD.gn b/api/BUILD.gn index 0c982a2ac6..9ced1f2e58 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -183,7 +183,9 @@ rtc_library("candidate") { "candidate.h", ] deps = [ + "../p2p:p2p_constants", "../rtc_base:checks", + "../rtc_base:crc32", "../rtc_base:ip_address", "../rtc_base:logging", "../rtc_base:network_constants", diff --git a/api/candidate.cc b/api/candidate.cc index 7718f94076..06b3156b96 100644 --- a/api/candidate.cc +++ b/api/candidate.cc @@ -11,6 +11,8 @@ #include "api/candidate.h" #include "absl/base/attributes.h" +#include "p2p/base/p2p_constants.h" +#include "rtc_base/crc32.h" #include "rtc_base/helpers.h" #include "rtc_base/ip_address.h" #include "rtc_base/logging.h" @@ -25,7 +27,7 @@ ABSL_CONST_INIT const absl::string_view RELAY_PORT_TYPE = "relay"; Candidate::Candidate() : id_(rtc::CreateRandomString(8)), - component_(0), + component_(ICE_CANDIDATE_COMPONENT_DEFAULT), priority_(0), type_(LOCAL_PORT_TYPE), network_type_(rtc::ADAPTER_TYPE_UNKNOWN), @@ -209,6 +211,43 @@ Candidate Candidate::ToSanitizedCopy(bool use_hostname_address, return copy; } +void Candidate::ComputeFoundation(const rtc::SocketAddress& base_address, + uint64_t tie_breaker) { + // https://www.rfc-editor.org/rfc/rfc5245#section-4.1.1.3 + // The foundation is an identifier, scoped within a session. Two candidates + // MUST have the same foundation ID when all of the following are true: + // + // o they are of the same type. + // o their bases have the same IP address (the ports can be different). + // o for reflexive and relayed candidates, the STUN or TURN servers used to + // obtain them have the same IP address. + // o they were obtained using the same transport protocol (TCP, UDP, etc.). + // + // Similarly, two candidates MUST have different foundations if their + // types are different, their bases have different IP addresses, the STUN or + // TURN servers used to obtain them have different IP addresses, or their + // transport protocols are different. + + rtc::StringBuilder sb; + sb << type_ << base_address.ipaddr().ToString() << protocol_ + << relay_protocol_; + + // https://www.rfc-editor.org/rfc/rfc5245#section-5.2 + // [...] it is possible for both agents to mistakenly believe they are + // controlled or controlling. To resolve this, each agent MUST select a random + // number, called the tie-breaker, uniformly distributed between 0 and (2**64) + // - 1 (that is, a 64-bit positive integer). This number is used in + // connectivity checks to detect and repair this case [...] + sb << rtc::ToString(tie_breaker); + foundation_ = rtc::ToString(rtc::ComputeCrc32(sb.Release())); +} + +void Candidate::ComputePrflxFoundation() { + RTC_DCHECK(is_prflx()); + RTC_DCHECK(!id_.empty()); + foundation_ = rtc::ToString(rtc::ComputeCrc32(id_)); +} + void Candidate::Assign(std::string& s, absl::string_view view) { // Assigning via a temporary object, like s = std::string(view), results in // binary size bloat. To avoid that, extract pointer and size from the diff --git a/api/candidate.h b/api/candidate.h index 0aa75aa192..77774ed5f7 100644 --- a/api/candidate.h +++ b/api/candidate.h @@ -74,10 +74,18 @@ class RTC_EXPORT Candidate { void set_component(int component) { component_ = component; } const std::string& protocol() const { return protocol_; } + + // Valid protocol values are: + // UDP_PROTOCOL_NAME, TCP_PROTOCOL_NAME, SSLTCP_PROTOCOL_NAME, + // TLS_PROTOCOL_NAME. void set_protocol(absl::string_view protocol) { Assign(protocol_, protocol); } // The protocol used to talk to relay. const std::string& relay_protocol() const { return relay_protocol_; } + + // Valid protocol values are: + // UDP_PROTOCOL_NAME, TCP_PROTOCOL_NAME, SSLTCP_PROTOCOL_NAME, + // TLS_PROTOCOL_NAME. void set_relay_protocol(absl::string_view protocol) { Assign(relay_protocol_, protocol); } @@ -169,7 +177,15 @@ class RTC_EXPORT Candidate { uint16_t network_id() const { return network_id_; } void set_network_id(uint16_t network_id) { network_id_ = network_id; } + // From RFC 5245, section-7.2.1.3: + // The foundation of the candidate is set to an arbitrary value, different + // from the foundation for all other remote candidates. + // Note: Use ComputeFoundation to populate this value. const std::string& foundation() const { return foundation_; } + + // TODO(tommi): Deprecate in favor of ComputeFoundation. + // For situations where serializing/deserializing a candidate is needed, + // the constructor can be used to inject a value for the foundation. void set_foundation(absl::string_view foundation) { Assign(foundation_, foundation); } @@ -221,6 +237,25 @@ class RTC_EXPORT Candidate { Candidate ToSanitizedCopy(bool use_hostname_address, bool filter_related_address) const; + // Computes and populates the `foundation()` field. + // Foundation: An arbitrary string that is the same for two candidates + // that have the same type, base IP address, protocol (UDP, TCP, + // etc.), and STUN or TURN server. If any of these are different, + // then the foundation will be different. Two candidate pairs with + // the same foundation pairs are likely to have similar network + // characteristics. Foundations are used in the frozen algorithm. + // A session wide (peerconnection) tie-breaker is applied to the foundation, + // adds additional randomness and must be the same for all candidates. + void ComputeFoundation(const rtc::SocketAddress& base_address, + uint64_t tie_breaker); + + // https://www.rfc-editor.org/rfc/rfc5245#section-7.2.1.3 + // Call to populate the foundation field for a new peer reflexive remote + // candidate. The type of the candidate must be "prflx". + // The foundation of the candidate is set to an arbitrary value, different + // from the foundation for all other remote candidates. + void ComputePrflxFoundation(); + private: // TODO(bugs.webrtc.org/13220): With C++17, we get a std::string assignment // operator accepting any object implicitly convertible to std::string_view, diff --git a/api/candidate_unittest.cc b/api/candidate_unittest.cc index e52d448e72..a74afdc871 100644 --- a/api/candidate_unittest.cc +++ b/api/candidate_unittest.cc @@ -29,9 +29,9 @@ TEST(CandidateTest, Id) { TEST(CandidateTest, Component) { Candidate c; - EXPECT_EQ(c.component(), 0); - c.set_component(ICE_CANDIDATE_COMPONENT_DEFAULT); EXPECT_EQ(c.component(), ICE_CANDIDATE_COMPONENT_DEFAULT); + c.set_component(ICE_CANDIDATE_COMPONENT_RTCP); + EXPECT_EQ(c.component(), ICE_CANDIDATE_COMPONENT_RTCP); } TEST(CandidateTest, TypeName) { @@ -53,4 +53,48 @@ TEST(CandidateTest, TypeName) { EXPECT_EQ(c.type(), RELAY_PORT_TYPE); } +TEST(CandidateTest, Foundation) { + Candidate c; + EXPECT_TRUE(c.foundation().empty()); + c.set_protocol("udp"); + c.set_relay_protocol("udp"); + + rtc::SocketAddress address("99.99.98.1", 1024); + c.set_address(address); + c.ComputeFoundation(c.address(), 1); + std::string foundation1 = c.foundation(); + EXPECT_FALSE(foundation1.empty()); + + // Change the tiebreaker. + c.ComputeFoundation(c.address(), 2); + std::string foundation2 = c.foundation(); + EXPECT_NE(foundation1, foundation2); + + // Provide a different base address. + address.SetIP("100.100.100.1"); + c.ComputeFoundation(address, 1); // Same tiebreaker as for foundation1. + foundation2 = c.foundation(); + EXPECT_NE(foundation1, foundation2); + + // Consistency check (just in case the algorithm ever changes to random!). + c.ComputeFoundation(c.address(), 1); + foundation2 = c.foundation(); + EXPECT_EQ(foundation1, foundation2); + + // Changing the protocol should affect the foundation. + auto prev_protocol = c.protocol(); + c.set_protocol("tcp"); + ASSERT_NE(prev_protocol, c.protocol()); + c.ComputeFoundation(c.address(), 1); + EXPECT_NE(foundation1, c.foundation()); + c.set_protocol(prev_protocol); + + // Changing the relay protocol should affect the foundation. + prev_protocol = c.relay_protocol(); + c.set_relay_protocol("tcp"); + ASSERT_NE(prev_protocol, c.relay_protocol()); + c.ComputeFoundation(c.address(), 1); + EXPECT_NE(foundation1, c.foundation()); +} + } // namespace cricket diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index 05db81ad96..1601b49e39 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -586,7 +586,6 @@ rtc_library("p2p_transport_channel") { "../logging:ice_log", "../rtc_base:async_packet_socket", "../rtc_base:checks", - "../rtc_base:crc32", "../rtc_base:dscp", "../rtc_base:event_tracer", "../rtc_base:ip_address", @@ -666,7 +665,6 @@ rtc_library("port") { "../rtc_base:byte_buffer", "../rtc_base:callback_list", "../rtc_base:checks", - "../rtc_base:crc32", "../rtc_base:dscp", "../rtc_base:event_tracer", "../rtc_base:ip_address", diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index 93a9a3f791..b601940667 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -1696,9 +1696,8 @@ void Connection::MaybeUpdateLocalCandidate(StunRequest* request, // Set the related address and foundation attributes before changing the // address. local_candidate_.set_related_address(local_candidate_.address()); - local_candidate_.set_foundation(port()->ComputeFoundation( - PRFLX_PORT_TYPE, local_candidate_.protocol(), - local_candidate_.relay_protocol(), local_candidate_.address())); + local_candidate_.ComputeFoundation(local_candidate_.address(), + port_->IceTiebreaker()); local_candidate_.set_priority(priority); local_candidate_.set_address(addr->GetAddress()); diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index eb3553b52a..b0e19f6091 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -34,7 +34,6 @@ #include "p2p/base/port.h" #include "p2p/base/wrapping_active_ice_controller.h" #include "rtc_base/checks.h" -#include "rtc_base/crc32.h" #include "rtc_base/experiments/struct_parameters_parser.h" #include "rtc_base/ip_address.h" #include "rtc_base/logging.h" @@ -1082,8 +1081,7 @@ void P2PTransportChannel::OnUnknownAddress(PortInterface* port, // From RFC 5245, section-7.2.1.3: // The foundation of the candidate is set to an arbitrary value, different // from the foundation for all other remote candidates. - remote_candidate.set_foundation( - rtc::ToString(rtc::ComputeCrc32(remote_candidate.id()))); + remote_candidate.ComputePrflxFoundation(); } // RFC5245, the agent constructs a pair whose local candidate is equal to diff --git a/p2p/base/port.cc b/p2p/base/port.cc index f49bfe524c..917167c864 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -26,7 +26,6 @@ #include "p2p/base/stun_request.h" #include "rtc_base/byte_buffer.h" #include "rtc_base/checks.h" -#include "rtc_base/crc32.h" #include "rtc_base/helpers.h" #include "rtc_base/ip_address.h" #include "rtc_base/logging.h" @@ -93,17 +92,6 @@ const char TCPTYPE_ACTIVE_STR[] = "active"; const char TCPTYPE_PASSIVE_STR[] = "passive"; const char TCPTYPE_SIMOPEN_STR[] = "so"; -std::string Port::ComputeFoundation(absl::string_view type, - absl::string_view protocol, - absl::string_view relay_protocol, - const rtc::SocketAddress& base_address) { - // TODO(bugs.webrtc.org/14605): ensure IceTiebreaker() is set. - rtc::StringBuilder sb; - sb << type << base_address.ipaddr().ToString() << protocol << relay_protocol - << rtc::ToString(IceTiebreaker()); - return rtc::ToString(rtc::ComputeCrc32(sb.Release())); -} - Port::Port(TaskQueueBase* thread, absl::string_view type, rtc::PacketSocketFactory* factory, @@ -260,22 +248,25 @@ void Port::AddAddress(const rtc::SocketAddress& address, bool is_final) { RTC_DCHECK_RUN_ON(thread_); - std::string foundation = - ComputeFoundation(type, protocol, relay_protocol, base_address); + // TODO(tommi): Set relay_protocol and optionally provide the base address + // to automatically compute the foundation in the ctor? It would be a good + // thing for the Candidate class to know the base address and keep it const. Candidate c(component_, protocol, address, 0U, username_fragment(), password_, - type, generation_, foundation, network_->id(), network_cost_); + type, generation_, "", network_->id(), network_cost_); + // Set the relay protocol before computing the foundation field. + c.set_relay_protocol(relay_protocol); + // TODO(bugs.webrtc.org/14605): ensure IceTiebreaker() is set. + c.ComputeFoundation(base_address, tiebreaker_); + c.set_priority( + c.GetPriority(type_preference, network_->preference(), relay_preference, + field_trials_->IsEnabled( + "WebRTC-IncreaseIceCandidatePriorityHostSrflx"))); #if RTC_DCHECK_IS_ON if (protocol == TCP_PROTOCOL_NAME && c.is_local()) { RTC_DCHECK(!tcptype.empty()); } #endif - - c.set_relay_protocol(relay_protocol); - c.set_priority( - c.GetPriority(type_preference, network_->preference(), relay_preference, - field_trials_->IsEnabled( - "WebRTC-IncreaseIceCandidatePriorityHostSrflx"))); c.set_tcptype(tcptype); c.set_network_name(network_->name()); c.set_network_type(network_->type()); diff --git a/p2p/base/port.h b/p2p/base/port.h index a3214af1b8..4021c250fb 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -374,18 +374,6 @@ class RTC_EXPORT Port : public PortInterface, public sigslot::has_slots<> { void GetStunStats(absl::optional* stats) override {} - // Foundation: An arbitrary string that is the same for two candidates - // that have the same type, base IP address, protocol (UDP, TCP, - // etc.), and STUN or TURN server. If any of these are different, - // then the foundation will be different. Two candidate pairs with - // the same foundation pairs are likely to have similar network - // characteristics. Foundations are used in the frozen algorithm. - std::string ComputeFoundation( - absl::string_view type, - absl::string_view protocol, - absl::string_view relay_protocol, - const rtc::SocketAddress& base_address) override; - protected: void UpdateNetworkCost() override; diff --git a/p2p/base/port_interface.h b/p2p/base/port_interface.h index fb8023b5dd..8a1d18d8ba 100644 --- a/p2p/base/port_interface.h +++ b/p2p/base/port_interface.h @@ -167,18 +167,6 @@ class PortInterface { // Called when the Connection discovers a local peer reflexive candidate. virtual void AddPrflxCandidate(const Candidate& local) = 0; - // Foundation: An arbitrary string that is the same for two candidates - // that have the same type, base IP address, protocol (UDP, TCP, - // etc.), and STUN or TURN server. If any of these are different, - // then the foundation will be different. Two candidate pairs with - // the same foundation pairs are likely to have similar network - // characteristics. Foundations are used in the frozen algorithm. - virtual std::string ComputeFoundation( - absl::string_view type, - absl::string_view protocol, - absl::string_view relay_protocol, - const rtc::SocketAddress& base_address) = 0; - protected: PortInterface(); virtual void UpdateNetworkCost() = 0; diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 98cbd2d7e3..351ab6de64 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -216,16 +216,13 @@ std::unique_ptr CreateFakeCandidate( uint32_t priority, const rtc::AdapterType underlying_type_for_vpn = rtc::ADAPTER_TYPE_UNKNOWN) { - std::unique_ptr candidate(new cricket::Candidate()); - candidate->set_address(rtc::SocketAddress(hostname, port)); - candidate->set_protocol(protocol); + std::unique_ptr candidate(new cricket::Candidate( + cricket::ICE_CANDIDATE_COMPONENT_RTP, protocol, + rtc::SocketAddress(hostname, port), priority, "iceusernamefragment", + "" /* pwd */, candidate_type, 0 /* generation */, "foundationIsAString")); + candidate->set_network_type(adapter_type); candidate->set_underlying_type_for_vpn(underlying_type_for_vpn); - candidate->set_type(candidate_type); - candidate->set_priority(priority); - // Defaults for testing. - candidate->set_foundation("foundationIsAString"); - candidate->set_username("iceusernamefragment"); return candidate; } @@ -498,6 +495,7 @@ class RTCStatsCollectorWrapper { CreateMockSender(media_type, track, ssrc, attachment_id, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); + EXPECT_CALL(*sender, SetCodecPreferences(_)); pc_->AddSender(sender); return sender; } @@ -574,6 +572,7 @@ class RTCStatsCollectorWrapper { voice_sender_info.local_stats[0].ssrc + 10, local_stream_ids); EXPECT_CALL(*rtp_sender, SetMediaChannel(_)).WillRepeatedly(Return()); EXPECT_CALL(*rtp_sender, Stop()); + EXPECT_CALL(*rtp_sender, SetCodecPreferences(_)); pc_->AddSender(rtp_sender); } @@ -612,6 +611,7 @@ class RTCStatsCollectorWrapper { video_sender_info.local_stats[0].ssrc + 10, local_stream_ids); EXPECT_CALL(*rtp_sender, SetMediaChannel(_)).WillRepeatedly(Return()); EXPECT_CALL(*rtp_sender, Stop()); + EXPECT_CALL(*rtp_sender, SetCodecPreferences(_)); pc_->AddSender(rtp_sender); } @@ -3092,6 +3092,7 @@ TEST_F(RTCStatsCollectorTest, RTCVideoSourceStatsCollectedForSenderWithTrack) { cricket::MEDIA_TYPE_VIDEO, video_track, kSsrc, kAttachmentId, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); + EXPECT_CALL(*sender, SetCodecPreferences(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3136,6 +3137,7 @@ TEST_F(RTCStatsCollectorTest, cricket::MEDIA_TYPE_VIDEO, video_track, kNoSsrc, kAttachmentId, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); + EXPECT_CALL(*sender, SetCodecPreferences(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3166,6 +3168,7 @@ TEST_F(RTCStatsCollectorTest, cricket::MEDIA_TYPE_VIDEO, video_track, kSsrc, kAttachmentId, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); + EXPECT_CALL(*sender, SetCodecPreferences(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3189,6 +3192,7 @@ TEST_F(RTCStatsCollectorTest, cricket::MEDIA_TYPE_AUDIO, /*track=*/nullptr, kSsrc, kAttachmentId, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); + EXPECT_CALL(*sender, SetCodecPreferences(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3541,6 +3545,7 @@ TEST_F(RTCStatsCollectorTest, cricket::MEDIA_TYPE_VIDEO, /*track=*/nullptr, kSsrc, kAttachmentId, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); + EXPECT_CALL(*sender, SetCodecPreferences(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3662,6 +3667,7 @@ TEST_F(RTCStatsCollectorTest, RtpIsMissingWhileSsrcIsZero) { rtc::scoped_refptr sender = CreateMockSender(cricket::MEDIA_TYPE_AUDIO, track, 0, 49, {}); EXPECT_CALL(*sender, Stop()); + EXPECT_CALL(*sender, SetCodecPreferences(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3679,11 +3685,11 @@ TEST_F(RTCStatsCollectorTest, DoNotCrashIfSsrcIsKnownButInfosAreStillMissing) { rtc::scoped_refptr sender = CreateMockSender(cricket::MEDIA_TYPE_AUDIO, track, 4711, 49, {}); EXPECT_CALL(*sender, Stop()); + EXPECT_CALL(*sender, SetCodecPreferences(_)); pc_->AddSender(sender); // We do not generate any matching voice_sender_info stats. rtc::scoped_refptr report = stats_->GetStatsReport(); - auto outbound_rtps = report->GetStatsOfType(); EXPECT_TRUE(outbound_rtps.empty()); }