Reland crrev.com/c/2529619

Made `RecordParsed::rdata_` nullable but missed a deref in
`RecordParsed::IsEqual()`.

Original change's description:
> Revert "Ignore unknown record types in DNS responses"
>
> This reverts commit e4620991.
>
> Reason for revert: Suspected for causing crashes on Windows/Intel.
> For more info see bug.
>
> Bug: 1148269
>
> Original change's description:
> > Ignore unknown record types in DNS responses
> >
> > Previously worked only for address requests. Other request types (e.g.
> > HTTPS), on seeing any unrecognized (and therefore unparsable) record
> > type in the response, would fail the whole response as malformed.
> >
> > Bug: 1147247
> > Change-Id: Icd9e5ab2e39cf09ffe7c3945b011ebf1d4b45778
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2529619
> > Commit-Queue: Eric Orth <ericorth@chromium.org>
> > Auto-Submit: Eric Orth <ericorth@chromium.org>
> > Reviewed-by: Matt Menke <mmenke@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#826462}
>
> TBR=mmenke@chromium.org,ericorth@chromium.org
>
> Change-Id: I03ea912d1a9c8a01be3d84a3f4d417c385f97c2d
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1147247
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533548
> Reviewed-by: Jamie Madill <jmadill@chromium.org>
> Commit-Queue: Jamie Madill <jmadill@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#826873}


Bug: 1148269,1147247
Fixed: 1147247
Change-Id: I312c095e519c62d730e5fb6165833727f5d913be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2534629
Commit-Queue: Eric Orth <ericorth@chromium.org>
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Auto-Submit: Eric Orth <ericorth@chromium.org>
Reviewed-by: Dan McArdle <dmcardle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827004}

net/dns/dns_test_util.cc

@@ -4,13 +4,19 @@
 
 #include "net/dns/dns_test_util.h"
 
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "base/big_endian.h"
 #include "base/bind.h"
+#include "base/check.h"
 #include "base/location.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/single_thread_task_runner.h"
 #include "base/sys_byteorder.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "net/base/io_buffer.h"
 #include "net/base/ip_address.h"
 #include "net/base/net_errors.h"
@@ -99,6 +105,23 @@ DnsResourceRecord BuildTestAddressRecord(std::string name,
       net::IPAddressToPackedString(ip), ttl);
 }
 
+DnsResourceRecord BuildTestTextRecord(std::string name,
+                                      std::vector<std::string> text_strings,
+                                      base::TimeDelta ttl) {
+  DCHECK(!text_strings.empty());
+
+  std::string rdata;
+  for (const std::string& text_string : text_strings) {
+    DCHECK(!text_string.empty());
+
+    rdata += base::checked_cast<unsigned char>(text_string.size());
+    rdata += text_string;
+  }
+
+  return BuildTestDnsRecord(std::move(name), dns_protocol::kTypeTXT,
+                            std::move(rdata));
+}
+
 DnsResourceRecord BuildTestHttpsAliasRecord(std::string name,
                                             base::StringPiece alias_name,
                                             base::TimeDelta ttl) {
@@ -215,18 +238,7 @@ DnsResponse BuildTestDnsTextResponse(
 
   std::vector<DnsResourceRecord> answers;
   for (std::vector<std::string>& text_record : text_records) {
-    DCHECK(!text_record.empty());
-
-    std::string rdata;
-    for (std::string text_string : text_record) {
-      DCHECK(!text_string.empty());
-
-      rdata += base::checked_cast<unsigned char>(text_string.size());
-      rdata += std::move(text_string);
-    }
-
-    answers.push_back(BuildTestDnsRecord(answer_name, dns_protocol::kTypeTXT,
-                                         std::move(rdata)));
+    answers.push_back(BuildTestTextRecord(answer_name, std::move(text_record)));
   }
 
   return BuildTestDnsResponse(std::move(name), dns_protocol::kTypeTXT, answers);

net/dns/dns_test_util.h

@@ -203,6 +203,11 @@ DnsResourceRecord BuildTestAddressRecord(
     const IPAddress& ip,
     base::TimeDelta ttl = base::TimeDelta::FromDays(1));
 
+DnsResourceRecord BuildTestTextRecord(
+    std::string name,
+    std::vector<std::string> text_strings,
+    base::TimeDelta ttl = base::TimeDelta::FromDays(1));
+
 DnsResourceRecord BuildTestHttpsAliasRecord(
     std::string name,
     base::StringPiece alias_name,

net/dns/host_resolver_manager_unittest.cc

@@ -7950,6 +7950,39 @@ TEST_F(HostResolverManagerDnsTest, TxtQuery) {
                                        bar_records.begin(), bar_records.end()));
 }
 
+// Test that TXT records can be extracted from a response that also contains
+// unrecognized record types.
+TEST_F(HostResolverManagerDnsTest, TxtQuery_MixedWithUnrecognizedType) {
+  std::vector<std::string> text_strings = {"foo"};
+
+  MockDnsClientRuleList rules;
+  rules.emplace_back(
+      "host", dns_protocol::kTypeTXT, false /* secure */,
+      MockDnsClientRule::Result(BuildTestDnsResponse(
+          "host", dns_protocol::kTypeTXT,
+          {BuildTestDnsRecord("host", 3u /* type */, "fake rdata 1"),
+           BuildTestTextRecord("host", std::move(text_strings)),
+           BuildTestDnsRecord("host", 3u /* type */, "fake rdata 2")})),
+      false /* delay */);
+
+  CreateResolver();
+  UseMockDnsClient(CreateValidDnsConfig(), std::move(rules));
+
+  HostResolver::ResolveHostParameters parameters;
+  parameters.dns_query_type = DnsQueryType::TXT;
+
+  ResolveHostResponseHelper response(resolver_->CreateRequest(
+      HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(),
+      parameters, resolve_context_.get(), resolve_context_->host_cache()));
+  EXPECT_THAT(response.result_error(), IsOk());
+  EXPECT_FALSE(response.request()->GetAddressResults());
+  EXPECT_FALSE(response.request()->GetHostnameResults());
+  EXPECT_FALSE(response.request()->GetExperimentalResultsForTesting());
+
+  EXPECT_THAT(response.request()->GetTextResults(),
+              testing::Optional(testing::ElementsAre("foo")));
+}
+
 TEST_F(HostResolverManagerDnsTest, TxtQuery_InvalidConfig) {
   set_allow_fallback_to_proctask(false);
   // Set empty DnsConfig.
@@ -8869,6 +8902,39 @@ TEST_F(HostResolverManagerDnsTest, HttpsQuery) {
   EXPECT_FALSE(response.request()->GetExperimentalResultsForTesting());
 }
 
+// Test that HTTPS records can be extracted from a response that also contains
+// unrecognized record types.
+TEST_F(HostResolverManagerDnsTest, HttpsQuery_MixedWithUnrecognizedType) {
+  const std::string kName = "https.test";
+
+  MockDnsClientRuleList rules;
+  std::vector<DnsResourceRecord> records = {
+      BuildTestDnsRecord(kName, 3u /* type */, "fake rdata 1"),
+      BuildTestHttpsAliasRecord(kName, "alias.test"),
+      BuildTestDnsRecord(kName, 3u /* type */, "fake rdata 2")};
+  rules.emplace_back(kName, dns_protocol::kTypeHttps, false /* secure */,
+                     MockDnsClientRule::Result(BuildTestDnsResponse(
+                         kName, dns_protocol::kTypeHttps, records)),
+                     false /* delay */);
+
+  CreateResolver();
+  UseMockDnsClient(CreateValidDnsConfig(), std::move(rules));
+
+  HostResolver::ResolveHostParameters parameters;
+  parameters.dns_query_type = DnsQueryType::HTTPS;
+
+  ResolveHostResponseHelper response(resolver_->CreateRequest(
+      HostPortPair(kName, 108), NetworkIsolationKey(), NetLogWithSource(),
+      parameters, resolve_context_.get(), resolve_context_->host_cache()));
+  // Experimental type, so does not affect overall result.
+  EXPECT_THAT(response.result_error(), IsError(ERR_NAME_NOT_RESOLVED));
+  EXPECT_FALSE(response.request()->GetAddressResults());
+  EXPECT_FALSE(response.request()->GetHostnameResults());
+  EXPECT_FALSE(response.request()->GetTextResults());
+  // Results never saved when overall result not OK.
+  EXPECT_FALSE(response.request()->GetExperimentalResultsForTesting());
+}
+
 TEST_F(HostResolverManagerDnsTest, HttpsQuery_MultipleResults) {
   const std::string kName = "https.test";
 

net/dns/record_parsed.cc

@@ -38,6 +38,7 @@ std::unique_ptr<const RecordParsed> RecordParsed::CreateFrom(
   if (!parser->ReadRecord(&record))
     return std::unique_ptr<const RecordParsed>();
 
+  bool unrecognized_type = false;
   switch (record.type) {
     case ARecordRdata::kType:
       rdata = ARecordRdata::Create(record.rdata, *parser);
@@ -71,10 +72,14 @@ std::unique_ptr<const RecordParsed> RecordParsed::CreateFrom(
       break;
     default:
       DVLOG(1) << "Unknown RData type for received record: " << record.type;
-      return std::unique_ptr<const RecordParsed>();
+      rdata = nullptr;
+      unrecognized_type = true;
+      break;
   }
 
-  if (!rdata.get())
+  // If a recognized type has a malformed rdata, consider the whole record
+  // malformed.
+  if (!rdata.get() && !unrecognized_type)
     return std::unique_ptr<const RecordParsed>();
 
   return std::unique_ptr<const RecordParsed>(
@@ -92,10 +97,9 @@ bool RecordParsed::IsEqual(const RecordParsed* other, bool is_mdns) const {
     other_klass &= dns_protocol::kMDnsClassMask;
   }
 
-  return name_ == other->name_ &&
-      klass == other_klass &&
-      type_ == other->type_ &&
-      rdata_->IsEqual(other->rdata_.get());
+  return name_ == other->name_ && klass == other_klass &&
+         type_ == other->type_ && !!rdata_ == !!other->rdata_ &&
+         (!rdata_ || rdata_->IsEqual(other->rdata_.get()));
 }
 
 }  // namespace net

net/dns/record_parsed.h

@@ -39,9 +39,16 @@ class NET_EXPORT_PRIVATE RecordParsed {
   template <class T> const T* rdata() const {
     if (T::kType != type_)
       return nullptr;
+
+    // Expect RData should always be parsed for recognized types.
+    DCHECK(rdata_);
+
     return static_cast<const T*>(rdata_.get());
   }
 
+  // Gets the rdata without casting to a known RData type.
+  const RecordRdata* rdata_for_testing() const { return rdata_.get(); }
+
   // Check if two records have the same data. Ignores time_created and ttl.
   // If |is_mdns| is true, ignore the top bit of the class
   // (the cache flush bit).

net/dns/record_parsed_unittest.cc

@@ -4,6 +4,9 @@
 
 #include "net/dns/record_parsed.h"
 
+#include <memory>
+
+#include "base/time/time.h"
 #include "net/dns/dns_response.h"
 #include "net/dns/dns_test_util.h"
 #include "net/dns/public/dns_protocol.h"
@@ -65,4 +68,108 @@ TEST(RecordParsedTest, CacheFlushBitCompare) {
   EXPECT_TRUE(record2->IsEqual(record1.get(), true));
 }
 
-}  //namespace net
+TEST(RecordParsedTest, ParseUnknownRdata) {
+  static const char kRecordData[] =
+      // NAME="foo.test"
+      "\003foo\004test\000"
+      // TYPE=MD (an obsolete type that will likely never be recognized by
+      // Chrome)
+      "\000\003"
+      // CLASS=IN
+      "\000\001"
+      // TTL=30 seconds
+      "\000\000\000\036"
+      // RDLENGTH=12 bytes
+      "\000\014"
+      // RDATA="garbage data"
+      "garbage data";
+  DnsRecordParser parser(kRecordData, sizeof(kRecordData) - 1, 0 /* offset */);
+
+  std::unique_ptr<const RecordParsed> record =
+      RecordParsed::CreateFrom(&parser, base::Time());
+
+  ASSERT_TRUE(record);
+  EXPECT_EQ(record->name(), "foo.test");
+  EXPECT_EQ(record->type(), 3u);
+  EXPECT_EQ(record->klass(), dns_protocol::kClassIN);
+  EXPECT_EQ(record->ttl(), 30u);
+  EXPECT_FALSE(record->rdata<ARecordRdata>());
+  EXPECT_FALSE(record->rdata_for_testing());
+}
+
+TEST(RecordParsedTest, EqualityHandlesUnknownRdata) {
+  static constexpr char kData[] =
+      // NAME="foo.test"
+      "\003foo\004test\000"
+      // TYPE=MD (an obsolete type that will likely never be recognized by
+      // Chrome)
+      "\000\003"
+      // CLASS=IN
+      "\000\001"
+      // TTL=30 seconds
+      "\000\000\000\036"
+      // RDLENGTH=12 bytes
+      "\000\014"
+      // RDATA="garbage data"
+      "garbage data"
+      // NAME="foo.test"
+      "\003foo\004test\000"
+      // TYPE=A
+      "\000\001"
+      // CLASS=IN
+      "\000\001"
+      // TTL=30 seconds
+      "\000\000\000\036"
+      // RDLENGTH=4 bytes
+      "\000\004"
+      // RDATA=8.8.8.8
+      "\010\010\010\010";
+  DnsRecordParser parser(kData, sizeof(kData) - 1, 0 /* offset */);
+
+  std::unique_ptr<const RecordParsed> unknown_record =
+      RecordParsed::CreateFrom(&parser, base::Time());
+  ASSERT_TRUE(unknown_record);
+  ASSERT_FALSE(unknown_record->rdata_for_testing());
+
+  std::unique_ptr<const RecordParsed> known_record =
+      RecordParsed::CreateFrom(&parser, base::Time());
+  ASSERT_TRUE(known_record);
+  ASSERT_TRUE(known_record->rdata_for_testing());
+
+  EXPECT_TRUE(
+      unknown_record->IsEqual(unknown_record.get(), false /* is_mdns */));
+  EXPECT_TRUE(
+      unknown_record->IsEqual(unknown_record.get(), true /* is_mdns */));
+  EXPECT_TRUE(known_record->IsEqual(known_record.get(), false /* is_mdns */));
+  EXPECT_TRUE(known_record->IsEqual(known_record.get(), true /* is_mdns */));
+  EXPECT_FALSE(
+      unknown_record->IsEqual(known_record.get(), false /* is_mdns */));
+  EXPECT_FALSE(unknown_record->IsEqual(known_record.get(), true /* is_mdns */));
+  EXPECT_FALSE(
+      known_record->IsEqual(unknown_record.get(), false /* is_mdns */));
+  EXPECT_FALSE(known_record->IsEqual(unknown_record.get(), true /* is_mdns */));
+}
+
+TEST(RecordParsedTest, RejectMalformedRdata) {
+  static const char kRecordData[] =
+      // NAME="foo.test"
+      "\003foo\004test\000"
+      // TYPE=PTR
+      "\000\014"
+      // CLASS=IN
+      "\000\001"
+      // TTL=31 seconds
+      "\000\000\000\037"
+      // RDLENGTH=1 byte
+      "\000\001"
+      // RDATA=truncated name
+      "\001";
+  DnsRecordParser parser(kRecordData, sizeof(kRecordData) - 1, 0 /* offset */);
+
+  std::unique_ptr<const RecordParsed> record =
+      RecordParsed::CreateFrom(&parser, base::Time());
+
+  EXPECT_FALSE(record);
+}
+
+}  // namespace net

net/dns/record_rdata.cc

@@ -48,8 +48,8 @@ bool RecordRdata::HasValidSize(const base::StringPiece& data, uint16_t type) {
     case dns_protocol::kTypeSOA:
       return true;
     default:
-      VLOG(1) << "Unsupported RDATA type.";
-      return false;
+      VLOG(1) << "Unrecognized RDATA type.";
+      return true;
   }
 }
 

net/dns/record_rdata.h

@@ -32,8 +32,9 @@ class NET_EXPORT RecordRdata {
  public:
   virtual ~RecordRdata() {}
 
-  // Return true if |data| represents RDATA in the wire format with a valid size
-  // for the give |type|.
+  // Return true if `data` represents RDATA in the wire format with a valid size
+  // for the give `type`. Always returns true for unrecognized `type`s as the
+  // size is never known to be invalid.
   static bool HasValidSize(const base::StringPiece& data, uint16_t type);
 
   virtual bool IsEqual(const RecordRdata* other) const = 0;