Make ERR_ICANN_NAME_COLLISION work for async DNS resolver.

net/dns/host_resolver_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/dns/host_resolver_impl.h"
 
 #if defined(OS_WIN)
 #include <Winsock2.h>
 #elif defined(OS_POSIX)
 #include <netdb.h>
@@ skipping 163 matching lines @@
 
 // ICANN uses this localhost address to indicate a name collision.
 //
 // The policy in Chromium is to fail host resolving if it resolves to
 // this special address.
 //
 // Not however that IP literals are exempt from this policy, so it is still
 // possible to navigate to http://127.0.53.53/ directly.
 //
 // For more details: https://www.icann.org/news/announcement-2-2014-08-01-en
-const uint8_t kIcanNameCollisionIp[] = {127, 0, 53, 53};
+bool ContainsIcannNameCollisionIp(const AddressList& addr_list) {
+  static const uint8_t kIcanNameCollisionIp[] = {127, 0, 53, 53};

[mmenke, 2016/12/13 19:24:15]

I thought not using static was preferred, to ensure no initializers?

[eroman, 2016/12/13 22:09:04]

Done.

Not sure (I have seen both instances in our code).
I have hoisted it back up to file-level scope to ensure consistency.

I think the C++ style guide says to use constexpr for function-level statics,
but didn't dig in too much.

+  for (const auto& endpoint : addr_list) {
+    const IPAddress& addr = endpoint.address();
+    if (addr.IsIPv4() && IPAddressStartsWith(addr, kIcanNameCollisionIp)) {
+      return true;
+    }
+  }
+  return false;
+}
 
 void UmaAsyncDnsResolveStatus(DnsResolveStatus result) {
   UMA_HISTOGRAM_ENUMERATION("AsyncDNS.ResolveStatus",
                             result,
                             RESOLVE_STATUS_MAX);
 }
 
 bool ResemblesNetBIOSName(const std::string& hostname) {
   return (hostname.size() < 16) && (hostname.find('.') == std::string::npos);
 }
@@ skipping 537 matching lines @@
                 const uint32_t attempt_number) {
     AddressList results;
     int os_error = 0;
     // Running on a worker task runner.
     int error = params_.resolver_proc->Resolve(key_.hostname,
                                                key_.address_family,
                                                key_.host_resolver_flags,
                                                &results,
                                                &os_error);
 
-    // Fail the resolution if the result contains 127.0.53.53. See the comment
-    // block of kIcanNameCollisionIp for details on why.
-    for (const auto& it : results) {
-      const IPAddress& cur = it.address();
-      if (cur.IsIPv4() && IPAddressStartsWith(cur, kIcanNameCollisionIp)) {
-        error = ERR_ICANN_NAME_COLLISION;
-        break;
-      }
-    }
-
     network_task_runner_->PostTask(
         FROM_HERE, base::Bind(&ProcTask::OnLookupComplete, this, results,
                               start_time, attempt_number, error, os_error));
   }
 
   // Makes next attempt if DoLookup() has not finished.
   void RetryIfNotComplete() {
     DCHECK(network_task_runner_->BelongsToCurrentThread());
 
     if (was_completed() || was_canceled())
@@ skipping 850 matching lines @@
         }
         UMA_HISTOGRAM_SPARSE_SLOWLY("AsyncDNS.ResolveError",
                                     std::abs(dns_task_error_));
         resolver_->OnDnsTaskResolve(dns_task_error_);
       } else {
         DNS_HISTOGRAM("AsyncDNS.FallbackFail", duration);
         UmaAsyncDnsResolveStatus(RESOLVE_STATUS_FAIL);
       }
     }
 
+    if (ContainsIcannNameCollisionIp(addr_list))
+      net_error = ERR_ICANN_NAME_COLLISION;
+
     base::TimeDelta ttl =
         base::TimeDelta::FromSeconds(kNegativeCacheEntryTTLSeconds);
     if (net_error == OK)
       ttl = base::TimeDelta::FromSeconds(kCacheEntryTTLSeconds);
 
     // Don't store the |ttl| in cache since it's not obtained from the server.
     CompleteRequests(
         HostCache::Entry(net_error, MakeAddressListForRequest(addr_list)),
         ttl);
   }
@@ skipping 70 matching lines @@
     }
 
     UmaAsyncDnsResolveStatus(RESOLVE_STATUS_DNS_SUCCESS);
     RecordTTL(ttl);
 
     resolver_->OnDnsTaskResolve(OK);
 
     base::TimeDelta bounded_ttl =
         std::max(ttl, base::TimeDelta::FromSeconds(kMinimumTTLSeconds));
 
+    if (ContainsIcannNameCollisionIp(addr_list))
+      net_error = ERR_ICANN_NAME_COLLISION;
+
     CompleteRequests(
         HostCache::Entry(net_error, MakeAddressListForRequest(addr_list), ttl),
         bounded_ttl);

[mmenke, 2016/12/13 19:24:16]

I know this is old behavior, but can we not pass the addr_list back on this
error?

[eroman, 2016/12/13 22:09:04]

Correct, I believe that addr_list shouldn't be used in cases where net_error !=
OK.

That applies to both the proc-based codepath, and the dns-task-based codepath.

I can take a look at this as a follow-up and see if special-casing makes sense
(i.e. don't call MakeAddressListForRequest).

My gut reaction is it isn't worth the extra code (and branch) to reduce doing
the small extra work in this failure case, but I'll check it out.

[mmenke, 2016/12/13 22:14:59]

I'm actually not at all concerned about perf here.  My concern is that we stick
the entries in the cache, and I can just think of all the potential ways things
could go bad from there.

I'm fine with another CL, if you think my paranoia is not completely
unwarranted.

[eroman, 2016/12/13 22:50:18]

That's a fair concern.
I will address in a follow-up.

   }
 
   void OnFirstDnsTransactionComplete() override {
     DCHECK(dns_task_->needs_two_transactions());
     DCHECK_EQ(dns_task_->needs_another_transaction(), is_queued());
     // No longer need to occupy two dispatcher slots.
     ReduceToOneJobSlot();
 
     // We already have a job slot at the dispatcher, so if the second
     // transaction hasn't started, reuse it now instead of waiting in the queue
@@ skipping 37 matching lines @@
       return;
     }
 
     net_log_.EndEventWithNetErrorCode(NetLogEventType::HOST_RESOLVER_IMPL_JOB,
                                       entry.error());
 
     resolver_->SchedulePersist();
 
     DCHECK(!requests_.empty());
 
-    if (entry.error() == OK) {
+    if (entry.error() == OK || entry.error() == ERR_ICANN_NAME_COLLISION) {
       // Record this histogram here, when we know the system has a valid DNS
       // configuration.
       UMA_HISTOGRAM_BOOLEAN("AsyncDNS.HaveDnsConfig",
                             resolver_->received_dns_config_);
     }
 
     bool did_complete = (entry.error() != ERR_NETWORK_CHANGED) &&
                         (entry.error() != ERR_HOST_RESOLVER_QUEUE_TOO_LARGE);
     if (did_complete) {
       resolver_->CacheResult(key_, entry, ttl);
@@ skipping 827 matching lines @@
   if (job_)
     job_->CancelRequest(this);
 }
 
 void HostResolverImpl::RequestImpl::ChangeRequestPriority(
     RequestPriority priority) {
   job_->ChangeRequestPriority(this, priority);
 }
 
 }  // namespace net