net: constrain AddressFamily in P2P resolver

Currently, P2PSocketManager has two methods:
  GetHostAddress(hostname, enable_mdns)
  GetHostAddressWithFamily(hostname, family, enable_mdns)
where the family is a bare integer. The network service requires the
family to be either AF_INET, AF_INET6, or AF_UNSPEC, but this is not
enforced in the Mojo interface anywhere. If client code calls the
network service with a value for family that is not one of these three,
the network service crashes.

This is obviously less than ideal, so this change switches the type of
the family from a bare int to a net::AddressFamily, which is an enum
that can only represent the legal values for this parameter. That pushes
error checking out into the clients of the Mojo interface, which are
better positioned to handle invalid values.

This change also merges GetHostAddress() and GetHostAddressWithFamily()
together by making the family parameter optional, so this interface now
has a single method:

  GetHostAddress(hostname, family?, enable_mdns)

and optionals are used to signal whether an address is present or not.
This is more idiomatic for Mojo, and removes a bunch of conditionals on
both the client and server sides of the interface.

Fixed: 413559012
Change-Id: I81d6927da6ebf341741bdc44346eca45779a6440
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6520938
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: Elly FJ <ellyjones@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1459741}

chrome/services/sharing/nearby/test_support/mock_webrtc_dependencies.h

@@ -36,18 +36,10 @@ class MockWebRtcDependencies
       void,
       GetHostAddress,
       (const std::string& host_name,
+       std::optional<net::AddressFamily> address_family,
        bool enable_mdns,
        network::mojom::P2PSocketManager::GetHostAddressCallback callback),
       (override));
-  MOCK_METHOD(
-      void,
-      GetHostAddressWithFamily,
-      (const std::string& host_name,
-       int address_family,
-       bool enable_mdns,
-       network::mojom::P2PSocketManager::GetHostAddressWithFamilyCallback
-           callback),
-      (override));
   MOCK_METHOD(
       void,
       CreateSocket,

chrome/services/sharing/webrtc/p2p_async_address_resolver.cc

@@ -10,6 +10,7 @@
 #include "base/functional/bind.h"
 #include "base/location.h"
 #include "components/webrtc/net_address_utils.h"
+#include "net/base/address_family.h"
 
 namespace sharing {
 
@@ -31,17 +32,16 @@ void P2PAsyncAddressResolver::Start(const webrtc::SocketAddress& host_name,
 
   state_ = STATE_SENT;
   done_callback_ = std::move(done_callback);
+
+  std::optional<net::AddressFamily> family = std::nullopt;
   if (address_family.has_value()) {
-    socket_manager_->GetHostAddressWithFamily(
-        host_name.hostname(), address_family.value(), /*enable_mdns=*/true,
-        base::BindOnce(&P2PAsyncAddressResolver::OnResponse,
-                       base::Unretained(this)));
-  } else {
-    socket_manager_->GetHostAddress(
-        host_name.hostname(), /*enable_mdns=*/true,
-        base::BindOnce(&P2PAsyncAddressResolver::OnResponse,
-                       base::Unretained(this)));
+    family = net::ToAddressFamily(*address_family);
   }
+
+  socket_manager_->GetHostAddress(
+      host_name.hostname(), family, /*enable_mdns=*/true,
+      base::BindOnce(&P2PAsyncAddressResolver::OnResponse,
+                     base::Unretained(this)));
 }
 
 void P2PAsyncAddressResolver::Cancel() {

net/base/address_family.h

@@ -41,7 +41,8 @@ NET_EXPORT AddressFamily GetAddressFamily(const IPAddress& address);
 // Maps the given AddressFamily to either AF_INET, AF_INET6 or AF_UNSPEC.
 NET_EXPORT int ConvertAddressFamily(AddressFamily address_family);
 
-// Maps AF_INET, AF_INET6 or AF_UNSPEC to an AddressFamily.
+// Maps AF_INET, AF_INET6 or AF_UNSPEC to an AddressFamily. Any other AF_ value
+// (or any other value) passed in results in NOTREACHED().
 NET_EXPORT AddressFamily ToAddressFamily(int family);
 
 }  // namespace net

services/network/network_context_unittest.cc

@@ -62,6 +62,7 @@
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "mojo/public/cpp/system/data_pipe_utils.h"
 #include "mojo/public/cpp/system/functions.h"
+#include "net/base/address_family.h"
 #include "net/base/cache_type.h"
 #include "net/base/features.h"
 #include "net/base/hash_value.h"
@@ -2048,7 +2049,7 @@ TEST_F(NetworkContextTest, P2PHostResolution) {
   base::RunLoop run_loop;
   std::vector<net::IPAddress> results;
   socket_manager->GetHostAddress(
-      kHostname, false /* enable_mdns */,
+      kHostname, std::nullopt /* address family */, false /* enable_mdns */,
       base::BindLambdaForTesting(
           [&](const std::vector<net::IPAddress>& addresses) {
             EXPECT_EQ(std::vector<net::IPAddress>{ip_address}, addresses);
@@ -2135,8 +2136,9 @@ TEST_F(NetworkContextTest, P2PHostResolutionWithFamily) {
     base::RunLoop run_loop;
     std::vector<net::IPAddress> results;
     // Expect IPv4 address when family passed as AF_INET.
-    socket_manager->GetHostAddressWithFamily(
-        kIPv4Hostname, AF_INET, false /* enable_mdns */,
+    socket_manager->GetHostAddress(
+        kIPv4Hostname, net::AddressFamily::ADDRESS_FAMILY_IPV4,
+        false /* enable_mdns */,
         base::BindLambdaForTesting(
             [&](const std::vector<net::IPAddress>& addresses) {
               EXPECT_EQ(std::vector<net::IPAddress>{ipv4_address}, addresses);
@@ -2149,8 +2151,9 @@ TEST_F(NetworkContextTest, P2PHostResolutionWithFamily) {
     base::RunLoop run_loop;
     std::vector<net::IPAddress> results;
     // Expect IPv6 address when family passed as AF_INET6.
-    socket_manager->GetHostAddressWithFamily(
-        kIPv6Hostname, AF_INET6, false /* enable_mdns */,
+    socket_manager->GetHostAddress(
+        kIPv6Hostname, net::AddressFamily::ADDRESS_FAMILY_IPV6,
+        false /* enable_mdns */,
         base::BindLambdaForTesting(
             [&](const std::vector<net::IPAddress>& addresses) {
               EXPECT_EQ(std::vector<net::IPAddress>{ipv6_address}, addresses);

services/network/p2p/socket_manager.cc

@@ -75,10 +75,6 @@ bool HasLocalTld(const std::string& host_name) {
   return EndsWith(host_name, kLocalTld, base::CompareCase::INSENSITIVE_ASCII);
 }
 
-net::DnsQueryType FamilyToDnsQueryType(int family) {
-  return net::AddressFamilyToDnsQueryType(net::ToAddressFamily(family));
-}
-
 }  // namespace
 
 DefaultLocalAddresses::DefaultLocalAddresses() = default;
@@ -92,7 +88,7 @@ class P2PSocketManager::DnsRequest {
       : resolver_(host_resolver), enable_mdns_(enable_mdns) {}
 
   void Resolve(const std::string& host_name,
-               std::optional<int> family,
+               std::optional<net::AddressFamily> family,
                const net::NetworkAnonymizationKey& network_anonymization_key,
                DoneCallback done_callback) {
     DCHECK(!done_callback.is_null());
@@ -123,7 +119,7 @@ class P2PSocketManager::DnsRequest {
 #endif  // ENABLE_MDNS
     }
     if (family.has_value()) {
-      parameters.dns_query_type = FamilyToDnsQueryType(family.value());
+      parameters.dns_query_type = net::AddressFamilyToDnsQueryType(*family);
     }
     request_ = resolver_->CreateRequest(host, network_anonymization_key,
                                         net::NetLogWithSource(), parameters);
@@ -387,24 +383,7 @@ void P2PSocketManager::StartNetworkNotifications(
 
 void P2PSocketManager::GetHostAddress(
     const std::string& host_name,
-    bool enable_mdns,
-    mojom::P2PSocketManager::GetHostAddressCallback callback) {
-  DoGetHostAddress(host_name, /*address_family=*/std::nullopt, enable_mdns,
-                   std::move(callback));
-}
-
-void P2PSocketManager::GetHostAddressWithFamily(
-    const std::string& host_name,
-    int address_family,
-    bool enable_mdns,
-    mojom::P2PSocketManager::GetHostAddressCallback callback) {
-  DoGetHostAddress(host_name, std::make_optional(address_family), enable_mdns,
-                   std::move(callback));
-}
-
-void P2PSocketManager::DoGetHostAddress(
-    const std::string& host_name,
-    std::optional<int> address_family,
+    std::optional<net::AddressFamily> address_family,
     bool enable_mdns,
     mojom::P2PSocketManager::GetHostAddressCallback callback) {
   auto request = std::make_unique<DnsRequest>(

services/network/p2p/socket_manager.h

@@ -126,11 +126,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) P2PSocketManager
       mojo::PendingRemote<mojom::P2PNetworkNotificationClient> client) override;
   void GetHostAddress(
       const std::string& host_name,
-      bool enable_mdns,
-      mojom::P2PSocketManager::GetHostAddressCallback callback) override;
-  void GetHostAddressWithFamily(
-      const std::string& host_name,
-      int address_family,
+      std::optional<net::AddressFamily> address_family,
       bool enable_mdns,
       mojom::P2PSocketManager::GetHostAddressCallback callback) override;
   void CreateSocket(
@@ -149,12 +145,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) P2PSocketManager
 
   void NetworkNotificationClientConnectionError();
 
-  void DoGetHostAddress(
-      const std::string& host_name,
-      std::optional<int> address_family,
-      bool enable_mdns,
-      mojom::P2PSocketManager::GetHostAddressCallback callback);
-
   void OnAddressResolved(
       DnsRequest* request,
       mojom::P2PSocketManager::GetHostAddressCallback callback,

services/network/public/cpp/BUILD.gn

@@ -192,6 +192,8 @@ component("cpp") {
 # need to depend on mojom_ip_address does not depend on the entire cpp_base.
 component("ip_address_mojom_support") {
   sources = [
+    "address_family_mojom_traits.cc",
+    "address_family_mojom_traits.h",
     "address_list_mojom_traits.cc",
     "address_list_mojom_traits.h",
     "alternate_protocol_usage_mojom_traits.cc",

services/network/public/cpp/address_family_mojom_traits.h

@@ -5,14 +5,16 @@
 #ifndef SERVICES_NETWORK_PUBLIC_CPP_ADDRESS_FAMILY_MOJOM_TRAITS_H_
 #define SERVICES_NETWORK_PUBLIC_CPP_ADDRESS_FAMILY_MOJOM_TRAITS_H_
 
+#include "base/component_export.h"
 #include "mojo/public/cpp/bindings/enum_traits.h"
 #include "net/base/address_family.h"
-#include "services/network/public/mojom/address_family.mojom.h"
+#include "services/network/public/mojom/address_family.mojom-shared.h"
 
 namespace mojo {
 
 template <>
-struct EnumTraits<network::mojom::AddressFamily, net::AddressFamily> {
+struct COMPONENT_EXPORT(NETWORK_CPP_IP_ADDRESS)
+    EnumTraits<network::mojom::AddressFamily, net::AddressFamily> {
   static network::mojom::AddressFamily ToMojom(
       net::AddressFamily address_family);
   static bool FromMojom(network::mojom::AddressFamily address_family,

services/network/public/mojom/BUILD.gn

@@ -108,6 +108,22 @@ mojom("mojom_ip_address") {
         "//services/network/public/cpp:ip_address_mojom_support",
       ]
     },
+    {
+      types = [
+        {
+          mojom = "network.mojom.AddressFamily"
+          cpp = "::net::AddressFamily"
+        },
+      ]
+      traits_headers = [
+        "//net/base/address_family.h",
+        "//services/network/public/cpp/address_family_mojom_traits.h",
+      ]
+      traits_public_deps = [
+        "//net",
+        "//services/network/public/cpp:ip_address_mojom_support",
+      ]
+    },
   ]
 
   cpp_typemaps = [
@@ -125,19 +141,6 @@ mojom("mojom_ip_address") {
         "//services/network/public/cpp:ip_address_mojom_support",
       ]
     },
-    {
-      types = [
-        {
-          mojom = "network.mojom.AddressFamily"
-          cpp = "::net::AddressFamily"
-        },
-      ]
-      traits_headers =
-          [ "//services/network/public/cpp/address_family_mojom_traits.h" ]
-      traits_sources =
-          [ "//services/network/public/cpp/address_family_mojom_traits.cc" ]
-      traits_public_deps = [ "//net" ]
-    },
     {
       types = [
         {

services/network/public/mojom/p2p.mojom

@@ -7,6 +7,7 @@ module network.mojom;
 import "mojo/public/mojom/base/unguessable_token.mojom";
 import "mojo/public/mojom/base/time.mojom";
 import "mojo/public/mojom/base/read_only_buffer.mojom";
+import "services/network/public/mojom/address_family.mojom";
 import "services/network/public/mojom/network_interface.mojom";
 import "services/network/public/mojom/ip_address.mojom";
 import "services/network/public/mojom/ip_endpoint.mojom";
@@ -63,15 +64,13 @@ interface P2PSocketManager {
   StartNetworkNotifications(
       pending_remote<P2PNetworkNotificationClient> client);
 
-  // Performs DNS hostname resolution.
-  GetHostAddress(string host_name, bool enable_mdns)
+  // Performs DNS hostname resolution, optionally with a specific address
+  // family.
+  GetHostAddress(string host_name,
+                 AddressFamily? address_family,
+                 bool enable_mdns)
       => (array<IPAddress> addresses);
 
-  // Performs DNS hostname resolution for a specific IP address family.
-  GetHostAddressWithFamily(string host_name,
-                           int32 address_family,
-                           bool enable_mdns) => (array<IPAddress> addresses);
-
   // Creates a P2PSocket
   CreateSocket(P2PSocketType type,
                IPEndPoint local_address,

third_party/blink/renderer/platform/p2p/DEPS

@@ -2,6 +2,7 @@ include_rules = [
     "+crypto/random.h",
     "+components/webrtc/net_address_utils.h",
     "+media/base/media_permission.h",
+    "+net/base/address_family.h",
     "+net/base/ip_address.h",
     "+net/base/ip_endpoint.h",
     "+net/base/network_change_notifier.h",

third_party/blink/renderer/platform/p2p/host_address_request.cc

@@ -10,6 +10,7 @@
 #include "base/feature_list.h"
 #include "base/location.h"
 #include "components/webrtc/net_address_utils.h"
+#include "net/base/address_family.h"
 #include "third_party/blink/public/common/features.h"
 #include "third_party/blink/renderer/platform/p2p/socket_dispatcher.h"
 #include "third_party/blink/renderer/platform/wtf/functional.h"
@@ -38,14 +39,15 @@ void P2PAsyncAddressResolver::Start(const webrtc::SocketAddress& host_name,
       blink::features::kWebRtcHideLocalIpsWithMdns);
   auto callback = WTF::BindOnce(&P2PAsyncAddressResolver::OnResponse,
                                 scoped_refptr<P2PAsyncAddressResolver>(this));
+
+  std::optional<net::AddressFamily> family = std::nullopt;
   if (address_family.has_value()) {
-    dispatcher_->GetP2PSocketManager()->GetHostAddressWithFamily(
-        String(host_name.hostname().data()), address_family.value(),
-        enable_mdns, std::move(callback));
-  } else {
-    dispatcher_->GetP2PSocketManager()->GetHostAddress(
-        String(host_name.hostname().data()), enable_mdns, std::move(callback));
+    family = net::ToAddressFamily(*address_family);
   }
+
+  dispatcher_->GetP2PSocketManager()->GetHostAddress(
+      String(host_name.hostname().data()), family, enable_mdns,
+      std::move(callback));
   dispatcher_ = nullptr;
 }