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

net/dns/dns_config_service_posix.cc

Index: net/dns/dns_config_service_posix.cc
diff --git a/net/dns/dns_config_service_posix.cc b/net/dns/dns_config_service_posix.cc
index ff2295e704a6aa3b1b6a80ac49d2d31185f9717f..a90ca7941818554ed3f8cd6490a745a5cc5bf6e3 100644
--- a/net/dns/dns_config_service_posix.cc
+++ b/net/dns/dns_config_service_posix.cc
@@ -10,6 +10,7 @@
 #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"
@@ -20,6 +21,61 @@
 #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)
@@ -31,14 +87,12 @@ 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:
@@ -76,6 +130,7 @@ class ConfigWatcher {
 
 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.
@@ -100,6 +155,32 @@ ConfigParsePosixResult ReadDnsConfig(DnsConfig* config) {
   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;
@@ -172,7 +253,18 @@ class DnsConfigServicePosix::ConfigReader : public SerialWorker {
   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_);
@@ -358,12 +450,16 @@ ConfigParsePosixResult ConvertResStateToDnsConfig(const struct __res_state& res,
   // 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;