Do not sanity check RR Name vs Question (#685)

It appears as though we should never sanity check the RR name vs the question name as some DNS servers may return results for alias records.

Fixes Bug: #683
Fix By: Brad House (@bradh352)

src/lib/ares__parse_into_addrinfo.c

@@ -81,7 +81,6 @@ ares_status_t ares__parse_into_addrinfo(const unsigned char *abuf, size_t alen,
   }
 
   for (i = 0; i < ancount; i++) {
-    const char          *rname = NULL;
     ares_dns_rec_type_t  rtype;
     const ares_dns_rr_t *rr =
       ares_dns_record_rr_get(dnsrec, ARES_SECTION_ANSWER, i);
@@ -91,13 +90,18 @@ ares_status_t ares__parse_into_addrinfo(const unsigned char *abuf, size_t alen,
     }
 
     rtype = ares_dns_rr_get_type(rr);
-    rname = ares_dns_rr_get_name(rr);
 
-    /* Old code did this hostname sanity check */
-    if ((rtype == ARES_REC_TYPE_A || rtype == ARES_REC_TYPE_AAAA) &&
-        strcasecmp(rname, hostname) != 0) {
-      continue;
-    }
+    /* Issue #683
+     * Old code did this hostname sanity check, however it appears this is
+     * flawed logic.  Other resolvers don't do this sanity check.  Leaving
+     * this code commented out for future reference.
+     *
+     * rname = ares_dns_rr_get_name(rr);
+     * if ((rtype == ARES_REC_TYPE_A || rtype == ARES_REC_TYPE_AAAA) &&
+     *     strcasecmp(rname, hostname) != 0) {
+     *   continue;
+     * }
+     */
 
     if (rtype == ARES_REC_TYPE_CNAME) {
       struct ares_addrinfo_cname *cname;

src/lib/ares_parse_ptr_reply.c

@@ -113,7 +113,6 @@ int ares_parse_ptr_reply(const unsigned char *abuf, int alen_int,
 
   /* Cycle through answers */
   for (i = 0; i < ancount; i++) {
-    const char          *rname = NULL;
     const ares_dns_rr_t *rr =
       ares_dns_record_rr_get(dnsrec, ARES_SECTION_ANSWER, i);
 
@@ -141,17 +140,20 @@ int ares_parse_ptr_reply(const unsigned char *abuf, int alen_int,
       continue;
     }
 
-    /* Old code compared the name in the rr to the ptrname, so we'll do that
-     * check here, but I'm not sure its necessary */
-    rname = ares_dns_rr_get_name(rr);
-    if (rname == NULL) {
-      /* Shouldn't be possible */
-      status = ARES_EBADRESP;
-      goto done;
-    }
-    if (strcasecmp(ptrname, rname) != 0) {
-      continue;
-    }
+    /* Issue #683
+     * Old code compared the name in the rr to the ptrname, but I think this
+     * is wrong since it was proven wrong for A & AAAA records.  Leaving
+     * this code commented out for future reference
+     *
+     * rname = ares_dns_rr_get_name(rr);
+     * if (rname == NULL) {
+     *   status = ARES_EBADRESP;
+     *   goto done;
+     * }
+     * if (strcasecmp(ptrname, rname) != 0) {
+     *   continue;
+     * }
+     */
 
     /* Save most recent PTR record as the hostname */
     hostname = ares_dns_rr_get_str(rr, ARES_RR_PTR_DNAME);

test/ares-test-parse-a.cc

@@ -312,13 +312,19 @@ TEST_F(LibraryTest, ParseAReplyErrors) {
   EXPECT_EQ(nullptr, host);
   pkt.add_question(new DNSQuestion("example.com", T_A));
 
-  // Question != answer
+  // Question != answer, this is ok as of Issue #683
   pkt.questions_.clear();
   pkt.add_question(new DNSQuestion("Axample.com", T_A));
   data = pkt.data();
-  EXPECT_EQ(ARES_ENODATA, ares_parse_a_reply(data.data(), (int)data.size(),
+  EXPECT_EQ(ARES_SUCCESS, ares_parse_a_reply(data.data(), (int)data.size(),
                                               &host, info, &count));
-  EXPECT_EQ(nullptr, host);
+  ASSERT_NE(nullptr, host);
+  std::stringstream ss;
+  ss << HostEnt(host);
+  EXPECT_EQ("{'Axample.com' aliases=[] addrs=[2.3.4.5]}", ss.str());
+  ares_free_hostent(host);
+  host = nullptr;
+
   pkt.questions_.clear();
   pkt.add_question(new DNSQuestion("example.com", T_A));
 

test/ares-test-parse-aaaa.cc

@@ -139,13 +139,19 @@ TEST_F(LibraryTest, ParseAaaaReplyErrors) {
   EXPECT_EQ(nullptr, host);
   pkt.add_question(new DNSQuestion("example.com", T_AAAA));
 
-  // Question != answer
+  // Question != answer, this is ok as of Issue #683
   pkt.questions_.clear();
   pkt.add_question(new DNSQuestion("Axample.com", T_AAAA));
   data = pkt.data();
-  EXPECT_EQ(ARES_ENODATA, ares_parse_aaaa_reply(data.data(), (int)data.size(),
+  EXPECT_EQ(ARES_SUCCESS, ares_parse_aaaa_reply(data.data(), (int)data.size(),
                                                 &host, info, &count));
-  EXPECT_EQ(nullptr, host);
+  ASSERT_NE(nullptr, host);
+  std::stringstream ss;
+  ss << HostEnt(host);
+  EXPECT_EQ("{'Axample.com' aliases=[] addrs=[0101:0101:0202:0202:0303:0303:0404:0404]}", ss.str());
+  ares_free_hostent(host);
+
+  host = nullptr;
   pkt.questions_.clear();
   pkt.add_question(new DNSQuestion("example.com", T_AAAA));
 

test/ares-test-parse-ptr.cc

@@ -163,13 +163,20 @@ TEST_F(LibraryTest, ParsePtrReplyErrors) {
                                                 addrv4, sizeof(addrv4), AF_INET, &host));
   pkt.add_question(new DNSQuestion("64.48.32.16.in-addr.arpa", T_PTR));
 
-  // Question != answer
+  // Question != answer, ok after #683
+  host = nullptr;
   pkt.questions_.clear();
   pkt.add_question(new DNSQuestion("99.48.32.16.in-addr.arpa", T_PTR));
   data = pkt.data();
-  EXPECT_EQ(ARES_ENODATA, ares_parse_ptr_reply(data.data(), (int)data.size(),
+  EXPECT_EQ(ARES_SUCCESS, ares_parse_ptr_reply(data.data(), (int)data.size(),
                                                addrv4, sizeof(addrv4), AF_INET, &host));
-  EXPECT_EQ(nullptr, host);
+  ASSERT_NE(nullptr, host);
+  std::stringstream ss;
+  ss << HostEnt(host);
+  EXPECT_EQ("{'other.com' aliases=[other.com] addrs=[16.32.48.64]}", ss.str());
+  ares_free_hostent(host);
+
+  host = nullptr;
   pkt.questions_.clear();
   pkt.add_question(new DNSQuestion("64.48.32.16.in-addr.arpa", T_PTR));