ICE: adjust priority of non-relay candidates Introduces a field trial WebRTC-IncreaseIceCandidatePriorityHostSrflx that adjusts the priority of non-relay candidates such that the STUN priority attribute calculated as (prflx-type-preference << 24) | (priority & 0x00FFFFFF) as described in https://www.rfc-editor.org/rfc/rfc5245#section-7.1.2.1 will satisfy the condition that the STUN priority of server-reflexive candidates will always be higher than the STUN priority of relay candidates. Previously this was not the case because the TURN relay preference was added to the local_preference for relay candidates, making it higher than the local_preference of srflx candidates gathered from the same interface. This led to cases where the resulting candidate pair priority of a srflx-relay pair was higher than the candidate pair priority of a srflx-srflx pair, i.e. using a TURN server in cases that work using a direct P2P connection. Whether the field trial is active can be observed by checking that priority-of-srflx-candidate&0x00FFFFFF is greater than priority-of-relay-candidate&0x00FFFFFF BUG=webrtc:13195,webrtc:5813,webrtc:15020 Change-Id: Ib91708fbe7310b6454f93158a45c9d77da009091 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/292700 Reviewed-by: Jonas Oreland <jonaso@webrtc.org> Reviewed-by: Harald Alvestrand <hta@webrtc.org> Commit-Queue: Philipp Hancke <phancke@microsoft.com> Cr-Commit-Position: refs/heads/main@{#40311}
diff --git a/api/candidate.cc b/api/candidate.cc index a14dda3..53a4826 100644 --- a/api/candidate.cc +++ b/api/candidate.cc
@@ -88,7 +88,8 @@ uint32_t Candidate::GetPriority(uint32_t type_preference, int network_adapter_preference, - int relay_preference) const { + int relay_preference, + bool adjust_local_preference) const { // RFC 5245 - 4.1.2.1. // priority = (2^24)*(type preference) + // (2^8)*(local preference) + @@ -106,11 +107,25 @@ // local preference = (NIC Type << 8 | Addr_Pref) + relay preference. // The relay preference is based on the number of TURN servers, the // first TURN server gets the highest preference. - int addr_pref = IPAddressPrecedence(address_.ipaddr()); int local_preference = ((network_adapter_preference << 8) | addr_pref) + relay_preference; + // Ensure that the added relay preference will not result in a relay candidate + // whose STUN priority attribute has a higher priority than a server-reflexive + // candidate. + // The STUN priority attribute is calculated as + // (peer-reflexive type preference) << 24 | (priority & 0x00FFFFFF) + // as described in + // https://www.rfc-editor.org/rfc/rfc5245#section-7.1.2.1 + // To satisfy that condition, add kMaxTurnServers to the local preference. + // This can not overflow the field width since the highest "NIC pref" + // assigned is kHighestNetworkPreference = 127 + RTC_DCHECK_LT(local_preference + kMaxTurnServers, 0x10000); + if (adjust_local_preference && relay_protocol_.empty()) { + local_preference += kMaxTurnServers; + } + return (type_preference << 24) | (local_preference << 8) | (256 - component_); }
diff --git a/api/candidate.h b/api/candidate.h index 281f2f0..15cd48c 100644 --- a/api/candidate.h +++ b/api/candidate.h
@@ -173,7 +173,8 @@ uint32_t GetPriority(uint32_t type_preference, int network_adapter_preference, - int relay_preference) const; + int relay_preference, + bool adjust_local_preference) const; bool operator==(const Candidate& o) const; bool operator!=(const Candidate& o) const;
diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 5c5ac63..3a1032a 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc
@@ -270,9 +270,11 @@ ComputeFoundation(type, protocol, relay_protocol, base_address); Candidate c(component_, protocol, address, 0U, username_fragment(), password_, type, generation_, foundation, network_->id(), network_cost_); - c.set_priority( - c.GetPriority(type_preference, network_->preference(), relay_preference)); 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_unittest.cc b/p2p/base/port_unittest.cc index 81096e6..1b1c156 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc
@@ -144,7 +144,8 @@ uint16_t min_port, uint16_t max_port, absl::string_view username_fragment, - absl::string_view password) + absl::string_view password, + const webrtc::FieldTrialsView* field_trials = nullptr) : Port(thread, type, factory, @@ -152,7 +153,8 @@ min_port, max_port, username_fragment, - password) {} + password, + field_trials) {} ~TestPort() {} // Expose GetStunMessage so that we can test it. @@ -802,12 +804,14 @@ STUN_ATTR_USERNAME, std::string(username))); return msg; } - std::unique_ptr<TestPort> CreateTestPort(const rtc::SocketAddress& addr, - absl::string_view username, - absl::string_view password) { - auto port = - std::make_unique<TestPort>(&main_, "test", &socket_factory_, - MakeNetwork(addr), 0, 0, username, password); + std::unique_ptr<TestPort> CreateTestPort( + const rtc::SocketAddress& addr, + absl::string_view username, + absl::string_view password, + const webrtc::FieldTrialsView* field_trials = nullptr) { + auto port = std::make_unique<TestPort>(&main_, "test", &socket_factory_, + MakeNetwork(addr), 0, 0, username, + password, field_trials); port->SignalRoleConflict.connect(this, &PortTest::OnRoleConflict); return port; } @@ -2626,6 +2630,44 @@ ASSERT_EQ(expected_priority_6bone, port->Candidates()[8].priority()); } +TEST_F(PortTest, TestComputeCandidatePriorityWithPriorityAdjustment) { + webrtc::test::ScopedKeyValueConfig field_trials( + "WebRTC-IncreaseIceCandidatePriorityHostSrflx/Enabled/"); + auto port = CreateTestPort(kLocalAddr1, "name", "pass", &field_trials); + port->SetIceTiebreaker(kTiebreakerDefault); + port->set_type_preference(90); + port->set_component(177); + port->AddCandidateAddress(SocketAddress("192.168.1.4", 1234)); + port->AddCandidateAddress(SocketAddress("2001:db8::1234", 1234)); + port->AddCandidateAddress(SocketAddress("fc12:3456::1234", 1234)); + port->AddCandidateAddress(SocketAddress("::ffff:192.168.1.4", 1234)); + port->AddCandidateAddress(SocketAddress("::192.168.1.4", 1234)); + port->AddCandidateAddress(SocketAddress("2002::1234:5678", 1234)); + port->AddCandidateAddress(SocketAddress("2001::1234:5678", 1234)); + port->AddCandidateAddress(SocketAddress("fecf::1234:5678", 1234)); + port->AddCandidateAddress(SocketAddress("3ffe::1234:5678", 1234)); + // These should all be: + // (90 << 24) | (([rfc3484 pref value] << 8) + kMaxTurnServers) | (256 - 177) + uint32_t expected_priority_v4 = 1509957199U + (kMaxTurnServers << 8); + uint32_t expected_priority_v6 = 1509959759U + (kMaxTurnServers << 8); + uint32_t expected_priority_ula = 1509962319U + (kMaxTurnServers << 8); + uint32_t expected_priority_v4mapped = expected_priority_v4; + uint32_t expected_priority_v4compat = 1509949775U + (kMaxTurnServers << 8); + uint32_t expected_priority_6to4 = 1509954639U + (kMaxTurnServers << 8); + uint32_t expected_priority_teredo = 1509952079U + (kMaxTurnServers << 8); + uint32_t expected_priority_sitelocal = 1509949775U + (kMaxTurnServers << 8); + uint32_t expected_priority_6bone = 1509949775U + (kMaxTurnServers << 8); + ASSERT_EQ(expected_priority_v4, port->Candidates()[0].priority()); + ASSERT_EQ(expected_priority_v6, port->Candidates()[1].priority()); + ASSERT_EQ(expected_priority_ula, port->Candidates()[2].priority()); + ASSERT_EQ(expected_priority_v4mapped, port->Candidates()[3].priority()); + ASSERT_EQ(expected_priority_v4compat, port->Candidates()[4].priority()); + ASSERT_EQ(expected_priority_6to4, port->Candidates()[5].priority()); + ASSERT_EQ(expected_priority_teredo, port->Candidates()[6].priority()); + ASSERT_EQ(expected_priority_sitelocal, port->Candidates()[7].priority()); + ASSERT_EQ(expected_priority_6bone, port->Candidates()[8].priority()); +} + // In the case of shared socket, one port may be shared by local and stun. // Test that candidates with different types will have different foundation. TEST_F(PortTest, TestFoundation) { @@ -2788,6 +2830,51 @@ #endif } +// Test the Connection priority is calculated correctly. +TEST_F(PortTest, TestConnectionPriorityWithPriorityAdjustment) { + webrtc::test::ScopedKeyValueConfig field_trials( + "WebRTC-IncreaseIceCandidatePriorityHostSrflx/Enabled/"); + auto lport = CreateTestPort(kLocalAddr1, "lfrag", "lpass", &field_trials); + lport->SetIceTiebreaker(kTiebreakerDefault); + lport->set_type_preference(cricket::ICE_TYPE_PREFERENCE_HOST); + + auto rport = CreateTestPort(kLocalAddr2, "rfrag", "rpass", &field_trials); + rport->SetIceTiebreaker(kTiebreakerDefault); + rport->set_type_preference(cricket::ICE_TYPE_PREFERENCE_RELAY_UDP); + lport->set_component(123); + lport->AddCandidateAddress(SocketAddress("192.168.1.4", 1234)); + rport->set_component(23); + rport->AddCandidateAddress(SocketAddress("10.1.1.100", 1234)); + + EXPECT_EQ(0x7E001E85U + (kMaxTurnServers << 8), + lport->Candidates()[0].priority()); + EXPECT_EQ(0x2001EE9U + (kMaxTurnServers << 8), + rport->Candidates()[0].priority()); + + // RFC 5245 + // pair priority = 2^32*MIN(G,D) + 2*MAX(G,D) + (G>D?1:0) + lport->SetIceRole(cricket::ICEROLE_CONTROLLING); + rport->SetIceRole(cricket::ICEROLE_CONTROLLED); + Connection* lconn = + lport->CreateConnection(rport->Candidates()[0], Port::ORIGIN_MESSAGE); +#if defined(WEBRTC_WIN) + EXPECT_EQ(0x2003EE9FC007D0BU, lconn->priority()); +#else + EXPECT_EQ(0x2003EE9FC007D0BLLU, lconn->priority()); +#endif + + lport->SetIceRole(cricket::ICEROLE_CONTROLLED); + rport->SetIceRole(cricket::ICEROLE_CONTROLLING); + Connection* rconn = + rport->CreateConnection(lport->Candidates()[0], Port::ORIGIN_MESSAGE); + RTC_LOG(LS_ERROR) << "RCONN " << rconn->priority(); +#if defined(WEBRTC_WIN) + EXPECT_EQ(0x2003EE9FC007D0AU, rconn->priority()); +#else + EXPECT_EQ(0x2003EE9FC007D0ALLU, rconn->priority()); +#endif +} + // Note that UpdateState takes into account the estimated RTT, and the // correctness of using `kMaxExpectedSimulatedRtt` as an upper bound of RTT in // the following tests depends on the link rate and the delay distriubtion
diff --git a/p2p/base/stun_port_unittest.cc b/p2p/base/stun_port_unittest.cc index def4951..eb7d25e 100644 --- a/p2p/base/stun_port_unittest.cc +++ b/p2p/base/stun_port_unittest.cc
@@ -334,6 +334,31 @@ EXPECT_EQ(kStunCandidatePriority, port()->Candidates()[0].priority()); } +TEST_F(StunPortWithMockDnsResolverTest, + TestPrepareAddressHostnameWithPriorityAdjustment) { + webrtc::test::ScopedKeyValueConfig field_trials( + "WebRTC-IncreaseIceCandidatePriorityHostSrflx/Enabled/"); + SetDnsResolverExpectations( + [](webrtc::MockAsyncDnsResolver* resolver, + webrtc::MockAsyncDnsResolverResult* resolver_result) { + EXPECT_CALL(*resolver, Start(kValidHostnameAddr, /*family=*/AF_INET, _)) + .WillOnce(InvokeArgument<2>()); + EXPECT_CALL(*resolver, result) + .WillRepeatedly(ReturnPointee(resolver_result)); + EXPECT_CALL(*resolver_result, GetError).WillOnce(Return(0)); + EXPECT_CALL(*resolver_result, GetResolvedAddress(AF_INET, _)) + .WillOnce(DoAll(SetArgPointee<1>(SocketAddress("127.0.0.1", 5000)), + Return(true))); + }); + CreateStunPort(kValidHostnameAddr); + PrepareAddress(); + EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock); + ASSERT_EQ(1U, port()->Candidates().size()); + EXPECT_TRUE(kLocalAddr.EqualIPs(port()->Candidates()[0].address())); + EXPECT_EQ(kStunCandidatePriority + (cricket::kMaxTurnServers << 8), + port()->Candidates()[0].priority()); +} + // Test that we handle hostname lookup failures properly. TEST_F(StunPortTestWithRealClock, TestPrepareAddressHostnameFail) { CreateStunPort(kBadHostnameAddr); @@ -684,4 +709,31 @@ EXPECT_EQ(kIPv6StunCandidatePriority, port()->Candidates()[0].priority()); } +// Same as before but with a field trial that changes the priority. +TEST_F(StunIPv6PortTestWithMockDnsResolver, + TestPrepareAddressHostnameWithPriorityAdjustment) { + webrtc::test::ScopedKeyValueConfig field_trials( + "WebRTC-IncreaseIceCandidatePriorityHostSrflx/Enabled/"); + SetDnsResolverExpectations( + [](webrtc::MockAsyncDnsResolver* resolver, + webrtc::MockAsyncDnsResolverResult* resolver_result) { + EXPECT_CALL(*resolver, + Start(kValidHostnameAddr, /*family=*/AF_INET6, _)) + .WillOnce(InvokeArgument<2>()); + EXPECT_CALL(*resolver, result) + .WillRepeatedly(ReturnPointee(resolver_result)); + EXPECT_CALL(*resolver_result, GetError).WillOnce(Return(0)); + EXPECT_CALL(*resolver_result, GetResolvedAddress(AF_INET6, _)) + .WillOnce(DoAll(SetArgPointee<1>(SocketAddress("::1", 5000)), + Return(true))); + }); + CreateStunPort(kValidHostnameAddr, &field_trials); + PrepareAddress(); + EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock); + ASSERT_EQ(1U, port()->Candidates().size()); + EXPECT_TRUE(kIPv6LocalAddr.EqualIPs(port()->Candidates()[0].address())); + EXPECT_EQ(kIPv6StunCandidatePriority + (cricket::kMaxTurnServers << 8), + port()->Candidates()[0].priority()); +} + } // namespace