[net] HostResolverImpl + DnsTransaction + DnsConfigService = Asynchronous DNS ready for experiments.

net/dns/dns_response.cc

Index: net/dns/dns_response.cc
diff --git a/net/dns/dns_response.cc b/net/dns/dns_response.cc
index 760bd5b68064dbc60c6a0f5b14b63e3e9d9b6f71..ae2d386ec8a9000db31d0ea46754170ce368c24f 100644
--- a/net/dns/dns_response.cc
+++ b/net/dns/dns_response.cc
@@ -5,6 +5,7 @@
 #include "net/dns/dns_response.h"
 
 #include "base/sys_byteorder.h"
+#include "net/base/address_list.h"
 #include "net/base/big_endian.h"
 #include "net/base/dns_util.h"
 #include "net/base/io_buffer.h"
@@ -220,4 +221,53 @@ const dns_protocol::Header* DnsResponse::header() const {
   return reinterpret_cast<const dns_protocol::Header*>(io_buffer_->data());
 }
 
+bool DnsResponse::ParseAddressList(AddressList* addr_list,
+                                   base::TimeDelta* ttl) const {
+  DCHECK(IsValid());
+  // DnsTransaction already verified that |response| matches the issued query
+  // in terms of query header. We still need to determine if there is a valid
+  // chain of CNAMEs from the query name to the RR owner name.
+
+  // Expected owner of record.
+  std::string expected_name = GetDottedName();

[cbentzel, 2012/02/14 02:01:54]

It looks like both DNSDomainToString and ParseName both omit trailing dot. Might
be worth mentioning since both need to be identical in behavior for this to work
due to the string comparison in the while loop.

+
+  uint16 expected_type = qtype();
+  size_t expected_size = (qtype() == dns_protocol::kTypeAAAA)

[cbentzel, 2012/02/14 02:01:54]

Could use expected_type instead of qtype().

+      ? kIPv6AddressSize : kIPv4AddressSize;

[cbentzel, 2012/02/14 02:01:54]

Do you want to make sure the qtype is only one of AAAA or A? I don't remember if
this is enforced elsewhere.

+
+  // getcanonname in eglibc returns the first owner name of an A or AAAA RR.
+  std::string canon_name;
+
+  uint32 ttl_sec = kuint32max;
+  IPAddressList ip_addresses;
+  DnsRecordParser parser = Parser();
+  DnsResourceRecord record;
+  while (parser.ParseRecord(&record)) {

[cbentzel, 2012/02/14 02:01:54]

Looks like this would accept a response for an A query of alpha like

alpha CNAME beta
beta A 18.85.0.1
beta CNAME gamma
gamma A 18.85.0.2

CNAMEs should probably be rejected once we process a non-CNAME entry?

[szym, 2012/02/14 15:56:24]

To answer both this and the next question:

I looked at evdns, glibc and c-ares. 

evdns:
http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/libevent/evdns.c
Only checks names in question. Does not check names in answers.

c-ares:
http://code.google.com/codesearch#5jiG3hroW30/trunk/libcurl.mod/src/ares/ares...
Assumes CNAMEs are in an ordered chain.

glibc:
http://code.google.com/codesearch#PfbwIaGvWwI/trunk/glibc-2.14/resolv/nss_dns...
??? I still have not successfully parsed through this.

Either way, if we think that a response like this is invalid, should we reject
it outright (ERR_DNS_SERVER_FAILED?) or accept part of it? I'm leaning towards
the former.

I did notice a BUG. This comparison below should ignore case.

[cbentzel, 2012/02/14 18:01:27]

I think that makes sense - this is unlikely to be something done intentionally.
You could even add something like malformed DNS response for an error.
 

+    if (record.name != expected_name) {
+      // We ignore records owned by unexpected name.
+      continue;
+    }
+    if (record.type == dns_protocol::kTypeCNAME) {
+      // New |hostname|, following the CNAME chain.
+      if (!parser.ParseName(record.rdata.begin(), &expected_name)) {

[cbentzel, 2012/02/14 02:01:54]

Are these guaranteed to always be in order of the chain, or could they be
reordered and still be valid? 

i.e. would a query for alpha and a response like

alpha CNAME beta
gamma CNAME delta
beta CNAME gamma
delta A 18.85.0.1

be valid, if not common?

+        return false;
+      }
+    } else if (record.type == expected_type &&
+               record.rdata.size() == expected_size) {
+      ip_addresses.push_back(IPAddressNumber(record.rdata.begin(),
+                                             record.rdata.end()));
+      ttl_sec = std::min(ttl_sec, record.ttl);

[cbentzel, 2012/02/14 02:01:54]

I thought TTL for all RRSet entries needed to be identical according to one of
the RFCs (will have to dig for which one). 

If you're going to be permissive, I'm OK, but may be worth calling out. Doing a
min seems correct.

[szym, 2012/02/14 17:44:39]

Not clear on this one. C-ares does not bundle the TTLs together. Instead it
returns one TTL per address fetched.

[cbentzel, 2012/02/14 18:01:27]

See http://tools.ietf.org/html/rfc2181#section-5 5.2

"Should a client receive a response containing RRs from an RRSet with differing
TTLs, it should treat this as an error."

It might be the case that this isn't actually obeyed in practice. 

[szym, 2012/02/14 18:07:20]

A good strategy could be to be strict initially, but have fine-grained
net_error. Then if the ProcTask succeeds when the DnsTask failed, we would
histogram the net_error returned from DnsTask. This would help us quantify how
often a presumably invalid response is accepted by the OS anyway.

[mmenke, 2012/02/14 18:46:00]

Agree that it would be great to have a number of network error codes for
different failures.

[mmenke, 2012/02/14 18:46:00]

I believe I saw a discussion somewhere (Maybe in a later RFC) athat had
suggestions on how to handle differing TTLs.  I believe it was either pick the
shortest or pick the first, can't remember which.

+      if (canon_name.empty())
+        canon_name = record.name;
+    }
+  }
+  if (ip_addresses.empty())
+    return false;
+
+  *addr_list = AddressList::CreateFromIPAddressList(ip_addresses,
+                                                    canon_name);
+  *ttl = base::TimeDelta::FromSeconds(ttl_sec);
+  return true;
+}
+
 }  // namespace net