Lookup STUN hostname using the network's IP family.

The current behaviour is to lookup using AF_UNSPEC, which leaves
the decision up to the getaddrinfo implementation, then filter to
resolved addresses matching the network family anyway.

Looking up using the network's family upfront avoids resolving to
an unusable address.

Bug: webrtc:14319, webrtc:14131
Change-Id: I4997452dc26aeb82e5d2890701721e7d477803a8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/270625
Commit-Queue: Sameer Vijaykar <samvi@google.com>
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Jonas Oreland <jonaso@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37753}

p2p/base/stun_port.cc

@@ -122,7 +122,10 @@ UDPPort::AddressResolver::AddressResolver(
     std::function<void(const rtc::SocketAddress&, int)> done_callback)
     : socket_factory_(factory), done_(std::move(done_callback)) {}
 
-void UDPPort::AddressResolver::Resolve(const rtc::SocketAddress& address) {
+void UDPPort::AddressResolver::Resolve(
+    const rtc::SocketAddress& address,
+    int family,
+    const webrtc::FieldTrialsView& field_trials) {
   if (resolvers_.find(address) != resolvers_.end())
     return;
 
@@ -133,12 +136,19 @@ void UDPPort::AddressResolver::Resolve(const rtc::SocketAddress& address) {
       pair = std::make_pair(address, std::move(resolver));
 
   resolvers_.insert(std::move(pair));
-  resolver_ptr->Start(address, [this, address] {
+  auto callback = [this, address] {
     ResolverMap::const_iterator it = resolvers_.find(address);
     if (it != resolvers_.end()) {
       done_(it->first, it->second->result().GetError());
     }
-  });
+  };
+  // Bug fix for STUN hostname resolution on IPv6.
+  // Field trial key reserved in bugs.webrtc.org/14334
+  if (field_trials.IsEnabled("WebRTC-IPv6NetworkResolutionFixes")) {
+    resolver_ptr->Start(address, family, std::move(callback));
+  } else {
+    resolver_ptr->Start(address, std::move(callback));
+  }
 }
 
 bool UDPPort::AddressResolver::GetResolvedAddress(
@@ -444,7 +454,7 @@ void UDPPort::ResolveStunAddress(const rtc::SocketAddress& stun_addr) {
 
   RTC_LOG(LS_INFO) << ToString() << ": Starting STUN host lookup for "
                    << stun_addr.ToSensitiveString();
-  resolver_->Resolve(stun_addr);
+  resolver_->Resolve(stun_addr, Network()->family(), field_trials());
 }
 
 void UDPPort::OnResolveResult(const rtc::SocketAddress& input, int error) {

p2p/base/stun_port.h

@@ -186,7 +186,9 @@ class UDPPort : public Port {
         rtc::PacketSocketFactory* factory,
         std::function<void(const rtc::SocketAddress&, int)> done_callback);
 
-    void Resolve(const rtc::SocketAddress& address);
+    void Resolve(const rtc::SocketAddress& address,
+                 int family,
+                 const webrtc::FieldTrialsView& field_trials);
     bool GetResolvedAddress(const rtc::SocketAddress& input,
                             int family,
                             rtc::SocketAddress* output) const;

p2p/base/stun_port_unittest.cc

@@ -128,17 +128,19 @@ class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> {
     network_.set_type(adapter_type);
   }
 
-  void CreateStunPort(const rtc::SocketAddress& server_addr) {
+  void CreateStunPort(const rtc::SocketAddress& server_addr,
+                      const webrtc::FieldTrialsView* field_trials = nullptr) {
     ServerAddresses stun_servers;
     stun_servers.insert(server_addr);
-    CreateStunPort(stun_servers);
+    CreateStunPort(stun_servers, field_trials);
   }
 
-  void CreateStunPort(const ServerAddresses& stun_servers) {
+  void CreateStunPort(const ServerAddresses& stun_servers,
+                      const webrtc::FieldTrialsView* field_trials = nullptr) {
     stun_port_ = cricket::StunPort::Create(
         rtc::Thread::Current(), socket_factory(), &network_, 0, 0,
         rtc::CreateRandomString(16), rtc::CreateRandomString(22), stun_servers,
-        absl::nullopt, &field_trials_);
+        absl::nullopt, field_trials);
     stun_port_->set_stun_keepalive_delay(stun_keepalive_delay_);
     // If `stun_keepalive_lifetime_` is negative, let the stun port
     // choose its lifetime from the network type.
@@ -152,8 +154,10 @@ class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> {
         this, &StunPortTestBase::OnCandidateError);
   }
 
-  void CreateSharedUdpPort(const rtc::SocketAddress& server_addr,
-                           rtc::AsyncPacketSocket* socket) {
+  void CreateSharedUdpPort(
+      const rtc::SocketAddress& server_addr,
+      rtc::AsyncPacketSocket* socket,
+      const webrtc::FieldTrialsView* field_trials = nullptr) {
     if (socket) {
       socket_.reset(socket);
     } else {
@@ -165,7 +169,7 @@ class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> {
     stun_port_ = cricket::UDPPort::Create(
         rtc::Thread::Current(), socket_factory(), &network_, socket_.get(),
         rtc::CreateRandomString(16), rtc::CreateRandomString(22), false,
-        absl::nullopt, &field_trials_);
+        absl::nullopt, field_trials);
     ASSERT_TRUE(stun_port_ != NULL);
     ServerAddresses stun_servers;
     stun_servers.insert(server_addr);
@@ -236,7 +240,6 @@ class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> {
 
  protected:
   cricket::IceCandidateErrorEvent error_event_;
-  webrtc::test::ScopedKeyValueConfig field_trials_;
 };
 
 class StunPortTestWithRealClock : public StunPortTestBase {};
@@ -361,7 +364,7 @@ TEST_F(StunPortTest, TestSharedSocketPrepareAddress) {
   EXPECT_TRUE(kLocalAddr.EqualIPs(port()->Candidates()[0].address()));
 }
 
-// Test that we still a get a local candidate with invalid stun server hostname.
+// Test that we still get a local candidate with invalid stun server hostname.
 // Also verifing that UDPPort can receive packets when stun address can't be
 // resolved.
 TEST_F(StunPortTestWithRealClock,
@@ -379,7 +382,8 @@ TEST_F(StunPortTestWithRealClock,
   // No crash is success.
 }
 
-// Test that the same address is added only once if two STUN servers are in use.
+// Test that the same address is added only once if two STUN servers are in
+// use.
 TEST_F(StunPortTest, TestNoDuplicatedAddressWithTwoStunServers) {
   ServerAddresses stun_servers;
   stun_servers.insert(kStunAddr1);
@@ -392,8 +396,8 @@ TEST_F(StunPortTest, TestNoDuplicatedAddressWithTwoStunServers) {
   EXPECT_EQ(port()->Candidates()[0].relay_protocol(), "");
 }
 
-// Test that candidates can be allocated for multiple STUN servers, one of which
-// is not reachable.
+// Test that candidates can be allocated for multiple STUN servers, one of
+// which is not reachable.
 TEST_F(StunPortTest, TestMultipleStunServersWithBadServer) {
   ServerAddresses stun_servers;
   stun_servers.insert(kStunAddr1);
@@ -640,4 +644,27 @@ TEST_F(StunIPv6PortTestWithMockDnsResolver, TestPrepareAddressHostname) {
   EXPECT_EQ(kIPv6StunCandidatePriority, port()->Candidates()[0].priority());
 }
 
+TEST_F(StunIPv6PortTestWithMockDnsResolver, TestPrepareAddressHostnameFamily) {
+  webrtc::test::ScopedKeyValueConfig field_trials(
+      "WebRTC-IPv6NetworkResolutionFixes/Enabled/");
+  SetDnsResolverExpectations(
+      [](webrtc::MockAsyncDnsResolver* resolver,
+         webrtc::MockAsyncDnsResolverResult* resolver_result) {
+        EXPECT_CALL(*resolver, Start(kValidHostnameAddr, 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, port()->Candidates()[0].priority());
+}
+
 }  // namespace

rtc_base/network.h

@@ -421,6 +421,9 @@ class RTC_EXPORT Network {
   // Returns the length, in bits, of this network's prefix.
   int prefix_length() const { return prefix_length_; }
 
+  // Returns the family for the network prefix.
+  int family() const { return prefix_.family(); }
+
   // `key_` has unique value per network interface. Used in sorting network
   // interfaces. Key is derived from interface name and it's prefix.
   std::string key() const { return key_; }

rtc_base/network_unittest.cc

@@ -334,6 +334,7 @@ TEST_F(NetworkTest, TestNetworkConstruct) {
   EXPECT_EQ("Test Network Adapter 1", ipv4_network1.description());
   EXPECT_EQ(IPAddress(0x12345600U), ipv4_network1.prefix());
   EXPECT_EQ(24, ipv4_network1.prefix_length());
+  EXPECT_EQ(AF_INET, ipv4_network1.family());
   EXPECT_FALSE(ipv4_network1.ignored());
 }
 
@@ -1125,6 +1126,7 @@ TEST_F(NetworkTest, TestIPv6Selection) {
   // Create a network with this prefix.
   Network ipv6_network("test_eth0", "Test NetworkAdapter", TruncateIP(ip, 64),
                        64);
+  EXPECT_EQ(AF_INET6, ipv6_network.family());
 
   // When there is no address added, it should return an unspecified
   // address.