Blank DNS names would result in ARES_ENOMEM due to bug in query cache

If a blank DNS name is used, the DNS query cache would fail due to an
invalid sanity check.  This can be legitimate such as:

adig -t SOA .

This fixes that situation as well as a few other spots that were
uncovered and adds a test case to validate the behavior to ensure
it won't regress in the future.

Fixes #858
Reported-By: Nodar Chkuaselidze (@nodech)
Authored-By: Brad House (@bradh352)

src/lib/ares__buf.c

@@ -215,10 +215,14 @@ ares_status_t ares__buf_append(ares__buf_t *buf, const unsigned char *data,
 {
   ares_status_t status;
 
-  if (data == NULL || data_len == 0) {
+  if (data == NULL && data_len != 0) {
     return ARES_EFORMERR;
   }
 
+  if (data_len == 0) {
+    return ARES_SUCCESS;
+  }
+
   status = ares__buf_ensure_space(buf, data_len);
   if (status != ARES_SUCCESS) {
     return status;

src/lib/ares_qcache.c

@@ -123,9 +123,11 @@ static char *ares__qcache_calc_key(const ares_dns_record_t *dnsrec)
       name_len--;
     }
 
-    status = ares__buf_append(buf, (const unsigned char *)name, name_len);
-    if (status != ARES_SUCCESS) {
-      goto fail;
+    if (name_len > 0) {
+      status = ares__buf_append(buf, (const unsigned char *)name, name_len);
+      if (status != ARES_SUCCESS) {
+        goto fail;
+      }
     }
   }
 

test/ares-test-internal.cc

@@ -908,7 +908,7 @@ TEST_F(LibraryTest, CatDomain) {
 TEST_F(LibraryTest, BufMisuse) {
   EXPECT_EQ(NULL, ares__buf_create_const(NULL, 0));
   ares__buf_reclaim(NULL);
-  EXPECT_NE(ARES_SUCCESS, ares__buf_append(NULL, NULL, 0));
+  EXPECT_NE(ARES_SUCCESS, ares__buf_append(NULL, NULL, 55));
   size_t len = 10;
   EXPECT_EQ(NULL, ares__buf_append_start(NULL, &len));
   EXPECT_EQ(NULL, ares__buf_append_start(NULL, NULL));

test/ares-test-mock.cc

@@ -617,6 +617,28 @@ TEST_P(MockChannelTest, SearchDomains) {
   EXPECT_EQ("{'www.third.gov' aliases=[] addrs=[2.3.4.5]}", ss.str());
 }
 
+// Issue #858
+TEST_P(CacheQueriesTest, BlankName) {
+  DNSPacket rsp;
+  rsp.set_response().set_aa()
+    .add_question(new DNSQuestion(".", T_SOA))
+    .add_answer(new DNSSoaRR(".", 600, "a.root-servers.net", "nstld.verisign-grs.com", 123456, 3600, 3600, 3600, 3600));
+  EXPECT_CALL(server_, OnRequest("", T_SOA))
+    .WillOnce(SetReply(&server_, &rsp));
+
+  QueryResult result;
+  ares_query_dnsrec(channel_, ".", ARES_CLASS_IN, ARES_REC_TYPE_SOA, QueryCallback, &result, NULL);
+  Process();
+  EXPECT_TRUE(result.done_);
+  EXPECT_EQ(0, result.timeouts_);
+
+  QueryResult cacheresult;
+  ares_query_dnsrec(channel_, ".", ARES_CLASS_IN, ARES_REC_TYPE_SOA, QueryCallback, &cacheresult, NULL);
+  Process();
+  EXPECT_TRUE(cacheresult.done_);
+  EXPECT_EQ(0, cacheresult.timeouts_);
+}
+
 // Relies on retries so is UDP-only
 TEST_P(MockUDPChannelTest, SearchDomainsWithResentReply) {
   DNSPacket nofirst;

test/ares-test.cc

@@ -977,6 +977,44 @@ void HostCallback(void *data, int status, int timeouts,
   if (verbose) std::cerr << "HostCallback(" << *result << ")" << std::endl;
 }
 
+std::ostream& operator<<(std::ostream& os, const AresDnsRecord& dnsrec) {
+  os << "{'";
+  /* XXX: Todo */
+  os << '}';
+  return os;
+}
+
+std::ostream& operator<<(std::ostream& os, const QueryResult& result) {
+  os << '{';
+  if (result.done_) {
+    os << StatusToString(result.status_);
+      if (result.dnsrec_.dnsrec_ != nullptr) {
+        os << " " << result.dnsrec_;
+      } else {
+        os << ", (no dnsrec)";
+      }
+  } else {
+    os << "(incomplete)";
+  }
+  os << '}';
+  return os;
+}
+
+void QueryCallback(void *data, ares_status_t status, size_t timeouts,
+                   const ares_dns_record_t *dnsrec) {
+  EXPECT_NE(nullptr, data);
+  if (data == nullptr)
+    return;
+
+  QueryResult* result = reinterpret_cast<QueryResult*>(data);
+  result->done_ = true;
+  result->status_ = status;
+  result->timeouts_ = timeouts;
+  if (dnsrec)
+    result->dnsrec_.SetDnsRecord(dnsrec);
+  if (verbose) std::cerr << "QueryCallback(" << *result << ")" << std::endl;
+}
+
 std::ostream& operator<<(std::ostream& os, const AddrInfoResult& result) {
   os << '{';
   if (result.done_ && result.ai_) {

test/ares-test.h

@@ -491,6 +491,51 @@ struct HostResult {
 
 std::ostream &operator<<(std::ostream &os, const HostResult &result);
 
+// C++ wrapper for ares_dns_record_t.
+struct AresDnsRecord {
+  ~AresDnsRecord()
+  {
+    ares_dns_record_destroy(dnsrec_);
+    dnsrec_ = NULL;
+  }
+
+  AresDnsRecord() : dnsrec_(NULL)
+  {
+  }
+
+  void SetDnsRecord(const ares_dns_record_t *dnsrec)
+  {
+    if (dnsrec_ != NULL) {
+      ares_dns_record_destroy(dnsrec_);
+    }
+    if (dnsrec == NULL) {
+      return;
+    }
+    dnsrec_ = ares_dns_record_duplicate(dnsrec);
+  }
+
+  ares_dns_record_t *dnsrec_ = NULL;
+};
+
+std::ostream &operator<<(std::ostream &os, const AresDnsRecord &result);
+
+// Structure that describes the result of an ares_host_callback invocation.
+struct QueryResult {
+  QueryResult() : done_(false), status_(ARES_SUCCESS), timeouts_(0)
+  {
+  }
+
+  // Whether the callback has been invoked.
+  bool          done_;
+  // Explicitly provided result information.
+  ares_status_t status_;
+  size_t        timeouts_;
+  // Contents of the ares_dns_record_t structure if provided
+  AresDnsRecord dnsrec_;
+};
+
+std::ostream &operator<<(std::ostream &os, const QueryResult &result);
+
 // Structure that describes the result of an ares_callback invocation.
 struct SearchResult {
   // Whether the callback has been invoked.
@@ -551,6 +596,8 @@ std::ostream &operator<<(std::ostream &os, const AddrInfoResult &result);
 // structures.
 void          HostCallback(void *data, int status, int timeouts,
                            struct hostent *hostent);
+void          QueryCallback(void *data, ares_status_t status, size_t timeouts,
+                            const ares_dns_record_t *dnsrec);
 void SearchCallback(void *data, int status, int timeouts, unsigned char *abuf,
                     int alen);
 void SearchCallbackDnsRec(void *data, ares_status_t status, size_t timeouts,