Detect domain-specific resolvers on OS X and disable DnsClient in such cases.

net/dns/dns_config_service_posix.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/dns_config_service_posix.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_path_watcher.h"
+#include "base/lazy_instance.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_util.h"
 #include "net/dns/dns_hosts.h"
 #include "net/dns/dns_protocol.h"
 #include "net/dns/notify_watcher_mac.h"
 #include "net/dns/serial_worker.h"
 
+#if defined(OS_MACOSX)
+#include <dlfcn.h>
+#include "third_party/apple_apsl/dnsinfo.h"

[mmenke, 2013/08/16 17:35:39]

nit:  Blank line between system and non-system headers.  Also between headers
and namespace.

[szym, 2013/08/17 05:08:51]

Done.

+namespace {
+
+// dnsinfo symbols are available via libSystem.dylib, but can also be present in
+// SystemConfiguration.framework. To avoid confusion, load them explicitly from
+// libSystem.dylib.
+class DnsInfoApi {
+ public:
+  typedef const char* (*dns_configuration_notify_key_t)();
+  typedef dns_config_t* (*dns_configuration_copy_t)();
+  typedef void (*dns_configuration_free_t)(dns_config_t*);
+
+  DnsInfoApi() {
+    handle_ = dlopen("/usr/lib/libSystem.dylib",
+                     RTLD_LAZY | RTLD_NOLOAD);
+    if (!handle_) return;

[mmenke, 2013/08/16 17:35:39]

Splirt onto two lines.

[szym, 2013/08/17 05:08:51]

Done.

+    dns_configuration_notify_key =

[mmenke, 2013/08/16 17:35:39]

These should be initialized to NULL.

[szym, 2013/08/17 05:08:51]

Done.

+      reinterpret_cast<dns_configuration_notify_key_t>(
+        dlsym(handle_, "dns_configuration_notify_key"));
+    dns_configuration_copy =
+      reinterpret_cast<dns_configuration_copy_t>(
+        dlsym(handle_, "dns_configuration_copy"));
+    dns_configuration_free =
+      reinterpret_cast<dns_configuration_free_t>(
+        dlsym(handle_, "dns_configuration_free"));
+  }
+  ~DnsInfoApi() {

[mmenke, 2013/08/16 17:35:39]

nit:  Blank line between functions.

[szym, 2013/08/17 05:08:51]

Done.

+    if (handle_) dlclose(handle_);

[mmenke, 2013/08/16 17:35:39]

nit:  Should be on two different lines.

[szym, 2013/08/17 05:08:51]

Done.

+  }
+
+  dns_configuration_notify_key_t dns_configuration_notify_key;
+  dns_configuration_copy_t dns_configuration_copy;
+  dns_configuration_free_t dns_configuration_free;
+
+ private:
+  void* handle_;
+};
+
+const DnsInfoApi& dns_info_api() {
+  base::LazyInstance<DnsInfoApi>::Leaky api = LAZY_INSTANCE_INITIALIZER;

[mmenke, 2013/08/16 17:35:39]

I can't find another instance where this is done - lazy instances are generally
just globals.  What's the motivation for the difference here?

[szym, 2013/08/16 17:48:27]

There is no need for it to be global, and I'd prefer to hide it so that the
struct cannot be accidentally modified. (see mmentovai's comment
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
)

But the bug here is that it should be static. 

[szym, 2013/08/17 05:08:51]

I added static.

+  return api.Get();
+}
+
+struct DnsConfigTDeleter {
+  inline void operator()(dns_config_t* ptr) const {
+    if (dns_info_api().dns_configuration_free)
+      dns_info_api().dns_configuration_free(ptr);
+  }
+};
+
+}  // namespace
+#endif

[mmenke, 2013/08/16 17:35:39]

nit:  #endif  // defined(OS_MACOSX)

[szym, 2013/08/17 05:08:51]

Done.

+
 namespace net {
 
 #if !defined(OS_ANDROID)
 namespace internal {
 
 namespace {
 
 const base::FilePath::CharType* kFilePathHosts =
     FILE_PATH_LITERAL("/etc/hosts");
 
 #if defined(OS_MACOSX)
-// From 10.7.3 configd-395.10/dnsinfo/dnsinfo.h
-static const char* kDnsNotifyKey =
-    "com.apple.system.SystemConfiguration.dns_configuration";
-
 class ConfigWatcher {
  public:
   bool Watch(const base::Callback<void(bool succeeded)>& callback) {
-    return watcher_.Watch(kDnsNotifyKey, callback);
+    if (!dns_info_api().dns_configuration_notify_key) return false;

[mmenke, 2013/08/16 17:35:39]

Split onto two lines.

[szym, 2013/08/17 05:08:51]

Done.

+    return watcher_.Watch(dns_info_api().dns_configuration_notify_key(),
+                          callback);
   }
 
  private:
   NotifyWatcherMac watcher_;
 };
 #else
 
 #ifndef _PATH_RESCONF  // Normally defined in <resolv.h>
 #define _PATH_RESCONF "/etc/resolv.conf"
 #endif
@@ skipping 17 matching lines @@
     callback_.Run(!error);
   }
 
   base::FilePathWatcher watcher_;
   CallbackType callback_;
 };
 #endif
 
 ConfigParsePosixResult ReadDnsConfig(DnsConfig* config) {
   ConfigParsePosixResult result;
+  config->unhandled_options = false;
 #if defined(OS_OPENBSD)
   // Note: res_ninit in glibc always returns 0 and sets RES_INIT.
   // res_init behaves the same way.
   memset(&_res, 0, sizeof(_res));
   if (res_init() == 0) {
     result = ConvertResStateToDnsConfig(_res, config);
   } else {
     result = CONFIG_PARSE_POSIX_RES_INIT_FAILED;
   }
 #else  // all other OS_POSIX
   struct __res_state res;
   memset(&res, 0, sizeof(res));
   if (res_ninit(&res) == 0) {
     result = ConvertResStateToDnsConfig(res, config);
   } else {
     result = CONFIG_PARSE_POSIX_RES_INIT_FAILED;
   }
   // Prefer res_ndestroy where available.
 #if defined(OS_MACOSX) || defined(OS_FREEBSD)
   res_ndestroy(&res);
 #else
   res_nclose(&res);
 #endif
 #endif
+
+#if defined(OS_MACOSX)
+  if (!dns_info_api().dns_configuration_copy)
+    return CONFIG_PARSE_POSIX_NO_DNSINFO;
+  scoped_ptr<dns_config_t, DnsConfigTDeleter> dns_config(
+      dns_info_api().dns_configuration_copy());
+  if (!dns_config)
+    return CONFIG_PARSE_POSIX_NO_DNSINFO;
+
+  // TODO(szym): Parse dns_config_t for resolvers rather than res_state.

[mmenke, 2013/08/16 17:35:39]

This comment isn't quite right - we actually do still get the resolvers from
res_state.

[szym, 2013/08/16 17:48:27]

We do right now, and that's why I left a TODO here to change it. dns_config_t is
the ground truth, because res_init basically copies data from dns_config_t to
__res_state.

[mmenke, 2013/08/16 18:24:19]

Right, missed the TODO.

+  // DnsClient can't handle domain-specific unscoped resolvers.
+  unsigned num_resolvers = 0;
+  for (int i = 0; i < dns_config->n_resolver; ++i) {

[mmenke, 2013/08/16 17:35:39]

Is there documentation on this data structure somewhere?  I'm not really sure of
the logic behind this.

[szym, 2013/08/16 17:48:27]

The source code is the only documentation. In particular, this is based on
http://opensource.apple.com/source/mDNSResponder/mDNSResponder-379.37/mDNSMac...
: ConfigResolvers

+    dns_resolver_t* resolver = dns_config->resolver[i];
+    if (!resolver->n_nameserver)
+      continue;
+    if (resolver->options && !strcmp(resolver->options, "mdns"))
+      continue;
+    ++num_resolvers;
+  }
+  if (num_resolvers > 1) {

[mmenke, 2013/08/16 18:24:19]

Rather then checking if there's more than one resolver, shouldn't we be checking
if there's any resolver with a non-null domain?  Or are we guaranteed the first
resolver will be for all domains, and other resolvers will not be?  It looks
like we get one nameserver per "resolver", so I'd think that in the case of
multiple nameservers, we'd have multiple resolvers, even if none are scoped to a
particular domain.

[szym, 2013/08/16 18:40:40]

That is what we need to check.
I'm not yet sure, but I'll need to figure it out before I can address the TODO
above.
dns_resolver_t is a configuration of a "resolver" but it has room for multiple
nameservers. |n_nameserver| is the number of items in the |nameserver| array of
sockaddr*.

The command "scutil --dns" dumps the config, and you'll see the different
resolvers, in the order they appear in dns_config_t.

The logic here is: if there is more than one non-mdns resolver (not nameserver),
then we can't handle it. We could check if that resolver has configured a
specific domain (it's possible to distinguish the "domain" from the "search
domain"), but I don't think it's necessary.

[mmenke, 2013/08/16 18:43:26]

Thanks for the explanation.  Seems reasonable, then.

+    LOG(WARNING) << "dns_config has unhandled options!";
+    config->unhandled_options = true;
+    return CONFIG_PARSE_POSIX_UNHANDLED_OPTIONS;
+  }
+#endif

[mmenke, 2013/08/16 17:35:39]

nit:  #endif  // defined(OS_MACOSX)

[szym, 2013/08/17 05:08:51]

Done.

   // Override timeout value to match default setting on Windows.
   config->timeout = base::TimeDelta::FromSeconds(kDnsTimeoutSeconds);
   return result;
 }
 
 }  // namespace
 
 class DnsConfigServicePosix::Watcher {
  public:
   explicit Watcher(DnsConfigServicePosix* service)
@@ skipping 52 matching lines @@
 // A SerialWorker that uses libresolv to initialize res_state and converts
 // it to DnsConfig.
 class DnsConfigServicePosix::ConfigReader : public SerialWorker {
  public:
   explicit ConfigReader(DnsConfigServicePosix* service)
       : service_(service), success_(false) {}
 
   virtual void DoWork() OVERRIDE {
     base::TimeTicks start_time = base::TimeTicks::Now();
     ConfigParsePosixResult result = ReadDnsConfig(&dns_config_);
-    success_ = (result == CONFIG_PARSE_POSIX_OK);
+    switch (result) {
+      case CONFIG_PARSE_POSIX_MISSING_OPTIONS:
+      case CONFIG_PARSE_POSIX_UNHANDLED_OPTIONS:
+        DCHECK(dns_config_.unhandled_options);

[mmenke, 2013/08/16 17:35:39]

Think it's worth a comment why this shouldn't happen.

[mmenke, 2013/08/16 17:51:30]

Sorry, misread this code.

+        // Fall through.
+      case CONFIG_PARSE_POSIX_OK:
+        success_ = true;
+        break;
+      default:
+        success_ = false;
+        break;
+    }
     UMA_HISTOGRAM_ENUMERATION("AsyncDNS.ConfigParsePosix",
                               result, CONFIG_PARSE_POSIX_MAX);
     UMA_HISTOGRAM_BOOLEAN("AsyncDNS.ConfigParseResult", success_);
     UMA_HISTOGRAM_TIMES("AsyncDNS.ConfigParseDuration",
                         base::TimeTicks::Now() - start_time);
   }
 
   virtual void OnWorkFinished() OVERRIDE {
     DCHECK(!IsCancelled());
     if (success_) {
@@ skipping 165 matching lines @@
   dns_config->timeout = base::TimeDelta::FromSeconds(res.retrans);
   dns_config->attempts = res.retry;
 #if defined(RES_ROTATE)
   dns_config->rotate = res.options & RES_ROTATE;
 #endif
   dns_config->edns0 = res.options & RES_USE_EDNS0;
 
   // The current implementation assumes these options are set. They normally
   // cannot be overwritten by /etc/resolv.conf
   unsigned kRequiredOptions = RES_RECURSE | RES_DEFNAMES | RES_DNSRCH;
-  if ((res.options & kRequiredOptions) != kRequiredOptions)
+  if ((res.options & kRequiredOptions) != kRequiredOptions) {
+    dns_config->unhandled_options = true;
     return CONFIG_PARSE_POSIX_MISSING_OPTIONS;
+  }
 
   unsigned kUnhandledOptions = RES_USEVC | RES_IGNTC | RES_USE_DNSSEC;
-  if (res.options & kUnhandledOptions)
+  if (res.options & kUnhandledOptions) {
+    dns_config->unhandled_options = true;
     return CONFIG_PARSE_POSIX_UNHANDLED_OPTIONS;
+  }
 
   if (dns_config->nameservers.empty())
     return CONFIG_PARSE_POSIX_NO_NAMESERVERS;
 
   // If any name server is 0.0.0.0, assume the configuration is invalid.
   // TODO(szym): Measure how often this happens. http://crbug.com/125599
   const IPAddressNumber kEmptyAddress(kIPv4AddressSize);
   for (unsigned i = 0; i < dns_config->nameservers.size(); ++i) {
     if (dns_config->nameservers[i].address() == kEmptyAddress)
       return CONFIG_PARSE_POSIX_NULL_ADDRESS;
@@ skipping 18 matching lines @@
   virtual void ReadNow() OVERRIDE {}
   virtual bool StartWatching() OVERRIDE { return false; }
 };
 // static
 scoped_ptr<DnsConfigService> DnsConfigService::CreateSystemService() {
   return scoped_ptr<DnsConfigService>(new StubDnsConfigService());
 }
 #endif
 
 }  // namespace net