Differentiate malformed DNS names and empty (root) name

Change the return type of DNSDomainToString() to
base::Optional<std::string>, so it can return nullopt on malformed and
empty string on root.  This is needed because HTTPS records can have
root names in some cases, so we need to be able to differentiate that
from actual malformed names.

While messing with the method interface, also fixed the camelcasing in
the method name, and changed the type to StringPiece value instead of
the unnecessary const&.

A subsequent CL will have more substantive fixes to make
DnsDomainToString() stricter to the standards validations we need, but
I wanted to get the mostly-mechanical stuff out of the way first in a
separate CL.

Bug: 1138620
Change-Id: I23d092051fe48826972a5fb5b3279dedf473ee1c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2495296
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Dan McArdle <dmcardle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820454}

net/dns/dns_query.cc

@@ -100,7 +100,10 @@ DnsQuery::DnsQuery(uint16_t id,
                    const OptRecordRdata* opt_rdata,
                    PaddingStrategy padding_strategy)
     : qname_size_(qname.size()) {
-  DCHECK(!DNSDomainToString(qname).empty());
+#if DCHECK_IS_ON()
+  base::Optional<std::string> dotted_name = DnsDomainToString(qname);
+  DCHECK(dotted_name && !dotted_name.value().empty());
+#endif  // DCHECK_IS_ON()
 
   size_t buffer_size = kHeaderSize + QuestionSize(qname_size_);
   base::Optional<OptRecordRdata> merged_opt_rdata =

net/dns/dns_response.cc

@@ -476,7 +476,7 @@ uint16_t DnsResponse::qtype() const {
 }
 
 std::string DnsResponse::GetDottedName() const {
-  return DNSDomainToString(qname());
+  return DnsDomainToString(qname()).value_or("");
 }
 
 DnsRecordParser DnsResponse::Parser() const {

net/dns/dns_transaction.cc

@@ -1368,9 +1368,11 @@ class DnsTransactionImpl : public DnsTransaction,
 
   // Begins query for the current name. Makes the first attempt.
   AttemptResult StartQuery() {
-    std::string dotted_qname = DNSDomainToString(qnames_.front());
-    net_log_.BeginEventWithStringParams(NetLogEventType::DNS_TRANSACTION_QUERY,
-                                        "qname", dotted_qname);
+    base::Optional<std::string> dotted_qname =
+        DnsDomainToString(qnames_.front());
+    net_log_.BeginEventWithStringParams(
+        NetLogEventType::DNS_TRANSACTION_QUERY, "qname",
+        dotted_qname.value_or("???MALFORMED_NAME???"));
 
     attempts_.clear();
     had_tcp_retry_ = false;

net/dns/dns_util.cc

@@ -8,12 +8,14 @@
 #include <limits.h>
 
 #include <cstring>
+#include <string>
 #include <unordered_map>
 #include <vector>
 
 #include "base/big_endian.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/optional.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
@@ -160,22 +162,22 @@ bool IsValidHostLabelCharacter(char c, bool is_first_char) {
          (c >= '0' && c <= '9') || (!is_first_char && c == '-') || c == '_';
 }
 
-std::string DNSDomainToString(const base::StringPiece& domain) {
+base::Optional<std::string> DnsDomainToString(base::StringPiece domain) {
   std::string ret;
 
   for (unsigned i = 0; i < domain.size() && domain[i]; i += domain[i] + 1) {
 #if CHAR_MIN < 0
     if (domain[i] < 0)
-      return std::string();
+      return base::nullopt;
 #endif
     if (domain[i] > kMaxLabelLength)
-      return std::string();
+      return base::nullopt;
 
     if (i)
       ret += ".";
 
     if (static_cast<unsigned>(domain[i]) + i + 1 > domain.size())
-      return std::string();
+      return base::nullopt;
 
     ret.append(domain.data() + i + 1, domain[i]);
   }

net/dns/dns_util.h

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/optional.h"
 #include "base/strings/string_piece.h"
 #include "base/time/time.h"
 #include "net/base/address_family.h"
@@ -63,9 +64,11 @@ NET_EXPORT_PRIVATE bool IsValidUnrestrictedDNSDomain(
 // not be NET_EXPORT_PRIVATE.
 NET_EXPORT_PRIVATE bool IsValidHostLabelCharacter(char c, bool is_first_char);
 
-// DNSDomainToString converts a domain in DNS format to a dotted string.
-// Excludes the dot at the end.
-NET_EXPORT std::string DNSDomainToString(const base::StringPiece& domain);
+// Converts a domain in DNS format to a dotted string. Excludes the dot at the
+// end. Assumes the standard terminating zero-length label at the end if not
+// included in the input. Returns nullopt on malformed input.
+NET_EXPORT base::Optional<std::string> DnsDomainToString(
+    base::StringPiece dns_name);
 
 // Return the expanded template when no variables have corresponding values.
 NET_EXPORT_PRIVATE std::string GetURLFromTemplateWithoutParameters(

net/dns/dns_util_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "net/dns/dns_util.h"
 
+#include "base/optional.h"
 #include "base/stl_util.h"
 #include "net/dns/public/dns_protocol.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -11,6 +12,8 @@
 
 namespace net {
 
+using testing::Eq;
+
 class DNSUtilTest : public testing::Test {
 };
 
@@ -79,19 +82,22 @@ TEST_F(DNSUtilTest, DNSDomainFromUnrestrictedDot) {
       &out));
 }
 
-TEST_F(DNSUtilTest, DNSDomainToString) {
-  EXPECT_EQ("", DNSDomainToString(IncludeNUL("")));
-  EXPECT_EQ("foo", DNSDomainToString(IncludeNUL("\003foo")));
-  EXPECT_EQ("foo.bar", DNSDomainToString(IncludeNUL("\003foo\003bar")));
-  EXPECT_EQ("foo.bar.uk",
-            DNSDomainToString(IncludeNUL("\003foo\003bar\002uk")));
+TEST_F(DNSUtilTest, DnsDomainToString) {
+  EXPECT_THAT(DnsDomainToString(IncludeNUL("")), testing::Optional(Eq("")));
+  EXPECT_THAT(DnsDomainToString(IncludeNUL("\003foo")),
+              testing::Optional(Eq("foo")));
+  EXPECT_THAT(DnsDomainToString(IncludeNUL("\003foo\003bar")),
+              testing::Optional(Eq("foo.bar")));
+  EXPECT_THAT(DnsDomainToString(IncludeNUL("\003foo\003bar\002uk")),
+              testing::Optional(Eq("foo.bar.uk")));
 
   // It should cope with a lack of root label.
-  EXPECT_EQ("foo.bar", DNSDomainToString("\003foo\003bar"));
+  EXPECT_THAT(DnsDomainToString("\003foo\003bar"),
+              testing::Optional(Eq("foo.bar")));
 
-  // Invalid inputs should return an empty string.
-  EXPECT_EQ("", DNSDomainToString(IncludeNUL("\x80")));
-  EXPECT_EQ("", DNSDomainToString("\x06"));
+  // Invalid inputs should return nullopt.
+  EXPECT_EQ(DnsDomainToString(IncludeNUL("\x80")), base::nullopt);
+  EXPECT_EQ(DnsDomainToString("\x06"), base::nullopt);
 }
 
 TEST_F(DNSUtilTest, IsValidDNSDomain) {

net/http/transport_security_state.cc

@@ -21,6 +21,7 @@
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/optional.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -1221,8 +1222,13 @@ bool TransportSecurityState::GetDynamicSTSState(const std::string& host,
     // An entry matches if it is either an exact match, or if it is a prefix
     // match and the includeSubDomains directive was included.
     if (i == 0 || j->second.include_subdomains) {
+      base::Optional<std::string> dotted_name =
+          DnsDomainToString(host_sub_chunk);
+      if (!dotted_name)
+        return false;
+
       *result = j->second;
-      result->domain = DNSDomainToString(host_sub_chunk);
+      result->domain = std::move(dotted_name).value();
       return true;
     }
   }
@@ -1262,8 +1268,13 @@ bool TransportSecurityState::GetDynamicPKPState(const std::string& host,
     // implement HPKP, so this logic is only used via AddHPKP(), reachable from
     // Cronet.
     if (i == 0 || j->second.include_subdomains) {
+      base::Optional<std::string> dotted_name =
+          DnsDomainToString(host_sub_chunk);
+      if (!dotted_name)
+        return false;
+
       *result = j->second;
-      result->domain = DNSDomainToString(host_sub_chunk);
+      result->domain = std::move(dotted_name).value();
       return true;
     }
 

services/network/mdns_responder.cc

@@ -6,6 +6,7 @@
 #include <cmath>
 #include <numeric>
 #include <queue>
+#include <string>
 #include <utility>
 
 #include "services/network/mdns_responder.h"
@@ -16,6 +17,7 @@
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/optional.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
@@ -966,11 +968,12 @@ void MdnsResponderManager::OnMdnsQueryReceived(
   // responder only provides APIs to create address records, and hence limited
   // to handle only such records. Once we have expanded the API surface to
   // include the service publishing, the handling logic should be unified.
-  const std::string qname = net::DNSDomainToString(query.qname());
+  const base::Optional<std::string> qname =
+      net::DnsDomainToString(query.qname());
   if (base::FeatureList::IsEnabled(
           features::kMdnsResponderGeneratedNameListing)) {
-    if (should_respond_to_generator_service_query_ &&
-        qname == kMdnsNameGeneratorServiceInstanceName) {
+    if (should_respond_to_generator_service_query_ && qname &&
+        qname.value() == kMdnsNameGeneratorServiceInstanceName) {
       HandleMdnsNameGeneratorServiceQuery(query, recv_socket_handler_id);
       return;
     }
@@ -1233,8 +1236,11 @@ void MdnsResponder::RemoveNameForAddress(
 void MdnsResponder::OnMdnsQueryReceived(const net::DnsQuery& query,
                                         uint16_t recv_socket_handler_id) {
   // Currently we only support a single question in DnsQuery.
-  std::string dotted_name_to_resolve = net::DNSDomainToString(query.qname());
-  auto it = name_addr_map_.find(dotted_name_to_resolve);
+  base::Optional<std::string> dotted_name_to_resolve =
+      net::DnsDomainToString(query.qname());
+  if (!dotted_name_to_resolve)
+    return;
+  auto it = name_addr_map_.find(dotted_name_to_resolve.value());
   if (it == name_addr_map_.end())
     return;