Add IsValidDNSDomain check to all HostResolverImpl::Resolve* calls.

https://chromium-review.googlesource.com/569298 added an extra check for
hostname validity before doing a DNS lookup, but it only covered the
main Resolve() path, not the two Resolve*FromCache ones. This prevents
bad domains from getting into the cache, but certain bad domain names
can still be resolved from the Resolve*FromCache calls without a DNS
lookup, like *.localhost domains.

This CL fixes that, and merges more common code between the different
resolution methods, so hopefully this will be less likely to regress in
the future.


Bug: 496468
Change-Id: I3ec16ae77a1f1a074e37a2c29a23d2423a4b42e0
Reviewed-on: https://chromium-review.googlesource.com/578207
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Miriam Gershenson <mgersh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488344}

net/dns/host_cache.h

@@ -39,6 +39,9 @@ class NET_EXPORT HostCache {
           address_family(address_family),
           host_resolver_flags(host_resolver_flags) {}
 
+    Key()
+        : address_family(ADDRESS_FAMILY_UNSPECIFIED), host_resolver_flags(0) {}
+
     bool operator<(const Key& other) const {
       // The order of comparisons of |Key| fields is arbitrary, thus
       // |address_family| and |host_resolver_flags| are compared before

net/dns/host_resolver_impl.cc

@@ -1962,26 +1962,14 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
   DCHECK_EQ(false, callback.is_null());
   DCHECK(out_req);
 
-  IPAddress ip_address;
-  IPAddress* ip_address_ptr = nullptr;
-  if (ip_address.AssignFromIPLiteral(info.hostname())) {
-    ip_address_ptr = &ip_address;
-  } else {
-    // Check that the caller supplied a valid hostname to resolve.
-    if (!IsValidDNSDomain(info.hostname()))
-      return ERR_NAME_NOT_RESOLVED;
-  }
-
   LogStartRequest(source_net_log, info);
 
-  // Build a key that identifies the request in the cache and in the
-  // outstanding jobs map.
-  Key key = GetEffectiveKeyForRequest(info, ip_address_ptr, source_net_log);
-
-  int rv = ResolveHelper(key, info, ip_address_ptr, addresses, false, nullptr,
-                         source_net_log);
+  Key key;
+  int rv = ResolveHelper(info, false, nullptr, source_net_log, addresses, &key);
   if (rv != ERR_DNS_CACHE_MISS) {
-    MaybeAddCacheHitCallback(key, info);
+    // TODO(mmenke): Remove this code. Nothing's using it.
+    if (rv == OK)
+      MaybeAddCacheHitCallback(key, info);
     LogFinishRequest(source_net_log, info, rv);
     RecordTotalTime(HaveDnsConfig(), info.is_speculative(), base::TimeDelta());
     return rv;
@@ -2085,13 +2073,26 @@ void HostResolverImpl::SetHaveOnlyLoopbackAddresses(bool result) {
   }
 }
 
-int HostResolverImpl::ResolveHelper(const Key& key,
-                                    const RequestInfo& info,
-                                    const IPAddress* ip_address,
-                                    AddressList* addresses,
+int HostResolverImpl::ResolveHelper(const RequestInfo& info,
                                     bool allow_stale,
                                     HostCache::EntryStaleness* stale_info,
-                                    const NetLogWithSource& source_net_log) {
+                                    const NetLogWithSource& source_net_log,
+                                    AddressList* addresses,
+                                    Key* key) {
+  IPAddress ip_address;
+  IPAddress* ip_address_ptr = nullptr;
+  if (ip_address.AssignFromIPLiteral(info.hostname())) {
+    ip_address_ptr = &ip_address;
+  } else {
+    // Check that the caller supplied a valid hostname to resolve.
+    if (!IsValidDNSDomain(info.hostname()))
+      return ERR_NAME_NOT_RESOLVED;
+  }
+
+  // Build a key that identifies the request in the cache and in the
+  // outstanding jobs map.
+  *key = GetEffectiveKeyForRequest(info, ip_address_ptr, source_net_log);
+
   DCHECK(allow_stale == !!stale_info);
   // The result of |getaddrinfo| for empty hosts is inconsistent across systems.
   // On Windows it gives the default interface's address, whereas on Linux it
@@ -2102,28 +2103,28 @@ int HostResolverImpl::ResolveHelper(const Key& key,
   }
 
   int net_error = ERR_UNEXPECTED;
-  if (ResolveAsIP(key, info, ip_address, &net_error, addresses)) {
+  if (ResolveAsIP(*key, info, ip_address_ptr, &net_error, addresses)) {
     MakeNotStale(stale_info);
     return net_error;
   }
-  if (ServeFromCache(key, info, &net_error, addresses, allow_stale,
+  if (ServeFromCache(*key, info, &net_error, addresses, allow_stale,
                      stale_info)) {
     source_net_log.AddEvent(NetLogEventType::HOST_RESOLVER_IMPL_CACHE_HIT,
                             addresses->CreateNetLogCallback());
     // |ServeFromCache()| will set |*stale_info| as needed.
-    RunCacheHitCallbacks(key, info);
+    RunCacheHitCallbacks(*key, info);
     return net_error;
   }
   // TODO(szym): Do not do this if nsswitch.conf instructs not to.
   // http://crbug.com/117655
-  if (ServeFromHosts(key, info, addresses)) {
+  if (ServeFromHosts(*key, info, addresses)) {
     source_net_log.AddEvent(NetLogEventType::HOST_RESOLVER_IMPL_HOSTS_HIT,
                             addresses->CreateNetLogCallback());
     MakeNotStale(stale_info);
     return OK;
   }
 
-  if (ServeLocalhost(key, info, addresses)) {
+  if (ServeLocalhost(*key, info, addresses)) {
     MakeNotStale(stale_info);
     return OK;
   }
@@ -2140,15 +2141,9 @@ int HostResolverImpl::ResolveFromCache(const RequestInfo& info,
   // Update the net log and notify registered observers.
   LogStartRequest(source_net_log, info);
 
-  IPAddress ip_address;
-  IPAddress* ip_address_ptr = nullptr;
-  if (ip_address.AssignFromIPLiteral(info.hostname()))
-    ip_address_ptr = &ip_address;
+  Key key;
+  int rv = ResolveHelper(info, false, nullptr, source_net_log, addresses, &key);
 
-  Key key = GetEffectiveKeyForRequest(info, ip_address_ptr, source_net_log);
-
-  int rv = ResolveHelper(key, info, ip_address_ptr, addresses, false, nullptr,
-                         source_net_log);
   LogFinishRequest(source_net_log, info, rv);
   return rv;
 }
@@ -2194,15 +2189,9 @@ int HostResolverImpl::ResolveStaleFromCache(
   // Update the net log and notify registered observers.
   LogStartRequest(source_net_log, info);
 
-  IPAddress ip_address;
-  IPAddress* ip_address_ptr = nullptr;
-  if (ip_address.AssignFromIPLiteral(info.hostname()))
-    ip_address_ptr = &ip_address;
-
-  Key key = GetEffectiveKeyForRequest(info, ip_address_ptr, source_net_log);
-
-  int rv = ResolveHelper(key, info, ip_address_ptr, addresses, true, stale_info,
-                         source_net_log);
+  Key key;
+  int rv =
+      ResolveHelper(info, true, stale_info, source_net_log, addresses, &key);
   LogFinishRequest(source_net_log, info, rv);
   return rv;
 }

net/dns/host_resolver_impl.h

@@ -194,19 +194,23 @@ class NET_EXPORT HostResolverImpl
   // incompatible, ERR_DNS_CACHE_MISS if entry was not found in cache and
   // HOSTS and is not localhost.
   //
+  // On success, the resulting addresses are written to |addresses|.
+  //
+  // On ERR_DNS_CACHE_MISS and OK, the cache key for the request is written to
+  // |key|. On other errors, it may not be.
+  //
   // If |allow_stale| is true, then stale cache entries can be returned.
   // |stale_info| must be non-null, and will be filled in with details of the
   // entry's staleness (if an entry is returned).
   //
   // If |allow_stale| is false, then stale cache entries will not be returned,
   // and |stale_info| must be null.
-  int ResolveHelper(const Key& key,
-                    const RequestInfo& info,
-                    const IPAddress* ip_address,
-                    AddressList* addresses,
+  int ResolveHelper(const RequestInfo& info,
                     bool allow_stale,
                     HostCache::EntryStaleness* stale_info,
-                    const NetLogWithSource& request_net_log);
+                    const NetLogWithSource& request_net_log,
+                    AddressList* addresses,
+                    Key* key);
 
   // Tries to resolve |key| as an IP, returns true and sets |net_error| if
   // succeeds, returns false otherwise.

net/dns/host_resolver_impl_unittest.cc

@@ -1411,24 +1411,30 @@ TEST_F(HostResolverImplTest, ResolveFromCache) {
 
 TEST_F(HostResolverImplTest, ResolveFromCacheInvalidName) {
   proc_->AddRuleForAllFamilies("foo,bar.com", "192.168.1.42");
-  proc_->SignalMultiple(1u);  // Need only one.
 
   HostResolver::RequestInfo info(HostPortPair("foo,bar.com", 80));
 
-  // First query will miss the cache.
-  EXPECT_EQ(ERR_DNS_CACHE_MISS,
-            CreateRequest(info, DEFAULT_PRIORITY)->ResolveFromCache());
+  // Query should be rejected before it makes it to the cache.
+  EXPECT_THAT(CreateRequest(info, DEFAULT_PRIORITY)->ResolveFromCache(),
+              IsError(ERR_NAME_NOT_RESOLVED));
 
-  // This time, we fetch normally.
+  // Query should be rejected without attempting to resolve it.
   EXPECT_THAT(CreateRequest(info, DEFAULT_PRIORITY)->Resolve(),
               IsError(ERR_NAME_NOT_RESOLVED));
   EXPECT_THAT(requests_[1]->WaitForResult(), IsError(ERR_NAME_NOT_RESOLVED));
+}
 
-  // Expect a cache miss, since the query could not have succeeded the first
-  // time.
+TEST_F(HostResolverImplTest, ResolveFromCacheInvalidNameLocalhost) {
+  HostResolver::RequestInfo info(HostPortPair("foo,bar.localhost", 80));
+
+  // Query should be rejected before it makes it to the localhost check.
   EXPECT_THAT(CreateRequest(info, DEFAULT_PRIORITY)->ResolveFromCache(),
-              IsError(ERR_DNS_CACHE_MISS));
-  EXPECT_FALSE(requests_[2]->HasOneAddress("192.168.1.42", 80));
+              IsError(ERR_NAME_NOT_RESOLVED));
+
+  // Query should be rejected without attempting to resolve it.
+  EXPECT_THAT(CreateRequest(info, DEFAULT_PRIORITY)->Resolve(),
+              IsError(ERR_NAME_NOT_RESOLVED));
+  EXPECT_THAT(requests_[1]->WaitForResult(), IsError(ERR_NAME_NOT_RESOLVED));
 }
 
 TEST_F(HostResolverImplTest, ResolveStaleFromCache) {