ares_getaddrinfo(): Fail faster on AF_UNSPEC if we've already received one address class (#551)

As per #541, when using AF_UNSPEC with ares_getaddrinfo() (and in turn with ares_gethostbynam()) if we receive a successful response for one address class, we should not allow the other address class to continue on with retries, just return the address class we have.

This will limit the overall query time to whatever timeout remains for the pending query for the other address class, it will not, however, terminate the other query as it may still prove to be successful (possibly coming in less than a millisecond later) and we'd want that result still. It just turns off additional error processing to get the result back quicker.

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

src/lib/ares_getaddrinfo.c

@@ -51,6 +51,7 @@
 #include "ares.h"
 #include "bitncmp.h"
 #include "ares_private.h"
+#include "ares_dns.h"
 
 #ifdef WATT32
 #undef WIN32
@@ -72,9 +73,12 @@ struct host_query
   const char *remaining_lookups; /* types of lookup we need to perform ("fb" by
                                     default, file and dns respectively) */
   struct ares_addrinfo *ai;      /* store results between lookups */
+  unsigned short qid_a;    /* qid for A request */
+  unsigned short qid_aaaa; /* qid for AAAA request */
   int remaining;   /* number of DNS answers waiting for */
   int next_domain; /* next search domain to try */
   int nodata_cnt; /* Track nodata responses to possibly override final result */
+
 };
 
 static const struct ares_addrinfo_hints default_hints = {
@@ -377,6 +381,7 @@ static void end_hquery(struct host_query *hquery, int status)
 {
   struct ares_addrinfo_node sentinel;
   struct ares_addrinfo_node *next;
+
   if (status == ARES_SUCCESS)
     {
       if (!(hquery->hints.ai_flags & ARES_AI_NOSORT) && hquery->ai->nodes)
@@ -565,17 +570,42 @@ static void next_lookup(struct host_query *hquery, int status)
     }
 }
 
+
+static void terminate_retries(struct host_query *hquery, unsigned short qid)
+{
+  unsigned short term_qid = (qid == hquery->qid_a)?hquery->qid_aaaa:hquery->qid_a;
+  ares_channel   channel  = hquery->channel;
+  struct query  *query    = NULL;
+
+  /* No other outstanding queries, nothing to do */
+  if (!hquery->remaining)
+    return;
+
+  query = ares__htable_stvp_get_direct(channel->queries_by_qid, term_qid);
+  if (query == NULL)
+    return;
+
+  query->no_retries = 1;
+}
+
+
 static void host_callback(void *arg, int status, int timeouts,
                           unsigned char *abuf, int alen)
 {
   struct host_query *hquery = (struct host_query*)arg;
   int addinfostatus = ARES_SUCCESS;
+  unsigned short qid = 0;
   hquery->timeouts += timeouts;
   hquery->remaining--;
 
   if (status == ARES_SUCCESS)
     {
       addinfostatus = ares__parse_into_addrinfo(abuf, alen, 1, hquery->port, hquery->ai);
+      if (addinfostatus == ARES_SUCCESS && alen >= HFIXEDSZ)
+        {
+          qid = DNS_HEADER_QID(abuf); /* Converts to host byte order */
+          terminate_retries(hquery, qid);
+        }
     }
 
   if (!hquery->remaining)
@@ -717,7 +747,7 @@ void ares_getaddrinfo(ares_channel channel,
     }
 
   /* Allocate and fill in the host query structure. */
-  hquery = ares_malloc(sizeof(struct host_query));
+  hquery = ares_malloc(sizeof(*hquery));
   if (!hquery)
     {
       ares_free(alias_name);
@@ -725,7 +755,7 @@ void ares_getaddrinfo(ares_channel channel,
       callback(arg, ARES_ENOMEM, 0, NULL);
       return;
     }
-
+  memset(hquery, 0, sizeof(*hquery));
   hquery->name = ares_strdup(name);
   ares_free(alias_name);
   if (!hquery->name)
@@ -743,11 +773,8 @@ void ares_getaddrinfo(ares_channel channel,
   hquery->callback = callback;
   hquery->arg = arg;
   hquery->remaining_lookups = channel->lookups;
-  hquery->timeouts = 0;
   hquery->ai = ai;
   hquery->next_domain = -1;
-  hquery->remaining = 0;
-  hquery->nodata_cnt = 0;
 
   /* Start performing lookups according to channel->lookups. */
   next_lookup(hquery, ARES_ECONNREFUSED /* initial error code */);
@@ -792,20 +819,26 @@ static int next_dns_lookup(struct host_query *hquery)
 
   if (s)
     {
+      /* NOTE: hquery may be invalidated during the call to ares_query_qid(),
+       *       so should not be referenced after this point */
       switch (hquery->hints.ai_family)
         {
           case AF_INET:
             hquery->remaining += 1;
-            ares_query(hquery->channel, s, C_IN, T_A, host_callback, hquery);
+            ares_query_qid(hquery->channel, s, C_IN, T_A, host_callback, hquery,
+                           &hquery->qid_a);
             break;
           case AF_INET6:
             hquery->remaining += 1;
-            ares_query(hquery->channel, s, C_IN, T_AAAA, host_callback, hquery);
+            ares_query_qid(hquery->channel, s, C_IN, T_AAAA, host_callback,
+                           hquery, &hquery->qid_aaaa);
             break;
           case AF_UNSPEC:
             hquery->remaining += 2;
-            ares_query(hquery->channel, s, C_IN, T_A, host_callback, hquery);
-            ares_query(hquery->channel, s, C_IN, T_AAAA, host_callback, hquery);
+            ares_query_qid(hquery->channel, s, C_IN, T_A, host_callback,
+                           hquery, &hquery->qid_a);
+            ares_query_qid(hquery->channel, s, C_IN, T_AAAA, host_callback,
+                           hquery, &hquery->qid_aaaa);
             break;
           default: break;
         }

src/lib/ares_private.h

@@ -248,6 +248,8 @@ struct query {
   int using_tcp;
   int error_status;
   int timeouts; /* number of timeouts we saw for this request */
+  int no_retries; /* do not perform any additional retries, this is set when
+                   * a query is to be canceled */
 };
 
 /* Per-server state for a query */
@@ -364,8 +366,20 @@ extern void (*ares_free)(void *ptr);
 int ares__timedout(struct timeval *now,
                    struct timeval *check);
 
-void ares__send_query(ares_channel channel, struct query *query,
-                      struct timeval *now);
+/* Returns one of the normal ares status codes like ARES_SUCCESS */
+int ares__send_query(ares_channel channel, struct query *query,
+                     struct timeval *now);
+
+/* Identical to ares_query, but returns a normal ares return code like
+ * ARES_SUCCESS, and can be passed the qid by reference which will be
+ * filled in on ARES_SUCCESS */
+int ares_query_qid(ares_channel channel, const char *name,
+                              int dnsclass, int type, ares_callback callback,
+                              void *arg, unsigned short *qid);
+/* Identical to ares_send() except returns normal ares return codes like
+ * ARES_SUCCESS */
+int ares_send_ex(ares_channel channel, const unsigned char *qbuf, int qlen,
+                 ares_callback callback, void *arg);
 void ares__close_connection(struct server_connection *conn);
 void ares__close_sockets(struct server_state *server);
 void ares__check_cleanup_conn(ares_channel channel, ares_socket_t fd);

src/lib/ares_process.c

@@ -71,7 +71,7 @@ static void process_answer(ares_channel channel, unsigned char *abuf,
 static void handle_error(struct server_connection *conn, struct timeval *now);
 static void skip_server(ares_channel channel, struct query *query,
                         struct server_state *server);
-static void next_server(ares_channel channel, struct query *query,
+static int next_server(ares_channel channel, struct query *query,
                         struct timeval *now);
 static int open_socket(ares_channel channel, struct server_state *server,
                        int is_tcp);
@@ -800,14 +800,18 @@ static void skip_server(ares_channel channel, struct query *query,
     }
 }
 
-static void next_server(ares_channel channel, struct query *query,
+static int next_server(ares_channel channel, struct query *query,
                         struct timeval *now)
 {
+  int status;
   /* We need to try each server channel->tries times. We have channel->nservers
    * servers to try. In total, we need to do channel->nservers * channel->tries
    * attempts. Use query->try to remember how many times we already attempted
-   * this query. Use modular arithmetic to find the next server to try. */
-  while (++(query->try_count) < (channel->nservers * channel->tries)) {
+   * this query. Use modular arithmetic to find the next server to try.
+   * A query can be requested be terminated at the next interval by setting
+   * query->no_retries */
+  while (++(query->try_count) < (channel->nservers * channel->tries) &&
+         !query->no_retries) {
     struct server_state *server;
 
     /* Move on to the next server. */
@@ -822,8 +826,7 @@ static void next_server(ares_channel channel, struct query *query,
         !(query->using_tcp &&
          (query->server_info[query->server].tcp_connection_generation ==
             server->tcp_connection_generation))) {
-      ares__send_query(channel, query, now);
-      return;
+      return ares__send_query(channel, query, now);
     }
 
     /* You might think that with TCP we only need one try. However, even
@@ -835,10 +838,12 @@ static void next_server(ares_channel channel, struct query *query,
   }
 
   /* If we are here, all attempts to perform query failed. */
+  status = query->error_status;
   end_query(channel, query, query->error_status, NULL, 0);
+  return status;
 }
 
-void ares__send_query(ares_channel channel, struct query *query,
+int ares__send_query(ares_channel channel, struct query *query,
                       struct timeval *now)
 {
   struct send_request *sendreq;
@@ -863,13 +868,12 @@ void ares__send_query(ares_channel channel, struct query *query,
         case ARES_ECONNREFUSED:
         case ARES_EBADFAMILY:
           skip_server(channel, query, server);
-          next_server(channel, query, now);
-          return;
+          return next_server(channel, query, now);
 
         /* Anything else is not retryable, likely ENOMEM */
         default:
           end_query(channel, query, err, NULL, 0);
-          return;
+          return err;
       }
     }
 
@@ -878,7 +882,7 @@ void ares__send_query(ares_channel channel, struct query *query,
     sendreq = ares_malloc(sizeof(struct send_request));
     if (!sendreq) {
       end_query(channel, query, ARES_ENOMEM, NULL, 0);
-      return;
+      return ARES_ENOMEM;
     }
     memset(sendreq, 0, sizeof(struct send_request));
     /* To make the common case fast, we avoid copies by using the query's
@@ -929,13 +933,12 @@ void ares__send_query(ares_channel channel, struct query *query,
         case ARES_ECONNREFUSED:
         case ARES_EBADFAMILY:
           skip_server(channel, query, server);
-          next_server(channel, query, now);
-          return;
+          return next_server(channel, query, now);
 
         /* Anything else is not retryable, likely ENOMEM */
         default:
           end_query(channel, query, err, NULL, 0);
-          return;
+          return err;
       }
       node = ares__llist_node_first(server->connections);
     }
@@ -944,8 +947,7 @@ void ares__send_query(ares_channel channel, struct query *query,
     if (socket_write(channel, conn->fd, query->qbuf, query->qlen) == -1) {
       /* FIXME: Handle EAGAIN here since it likely can happen. */
       skip_server(channel, query, server);
-      next_server(channel, query, now);
-      return;
+      return next_server(channel, query, now);
     }
   }
 
@@ -983,7 +985,7 @@ void ares__send_query(ares_channel channel, struct query *query,
   query->node_queries_by_timeout = ares__slist_insert(channel->queries_by_timeout, query);
   if (!query->node_queries_by_timeout) {
     end_query(channel, query, ARES_ENOMEM, NULL, 0);
-    return;
+    return ARES_ENOMEM;
   }
 
   /* Keep track of queries bucketed by connection, so we can process errors
@@ -993,6 +995,7 @@ void ares__send_query(ares_channel channel, struct query *query,
     ares__llist_insert_last(conn->queries_to_conn, query);
   query->conn = conn;
   conn->total_queries++;
+  return ARES_SUCCESS;
 }
 
 /*

src/lib/ares_query.c

@@ -52,8 +52,9 @@ static unsigned short generate_unique_id(ares_channel channel)
   return (unsigned short)id;
 }
 
-void ares_query(ares_channel channel, const char *name, int dnsclass,
-                int type, ares_callback callback, void *arg)
+int ares_query_qid(ares_channel channel, const char *name,
+                   int dnsclass, int type, ares_callback callback,
+                   void *arg, unsigned short *qid)
 {
   struct qquery *qquery;
   unsigned char *qbuf;
@@ -68,7 +69,7 @@ void ares_query(ares_channel channel, const char *name, int dnsclass,
     {
       if (qbuf != NULL) ares_free(qbuf);
       callback(arg, status, 0, NULL, 0);
-      return;
+      return status;
     }
 
   /* Allocate and fill in the query structure. */
@@ -77,16 +78,28 @@ void ares_query(ares_channel channel, const char *name, int dnsclass,
     {
       ares_free_string(qbuf);
       callback(arg, ARES_ENOMEM, 0, NULL, 0);
-      return;
+      return ARES_ENOMEM;
     }
   qquery->callback = callback;
   qquery->arg = arg;
 
   /* Send it off.  qcallback will be called when we get an answer. */
-  ares_send(channel, qbuf, qlen, qcallback, qquery);
+  status = ares_send_ex(channel, qbuf, qlen, qcallback, qquery);
   ares_free_string(qbuf);
+
+  if (status == ARES_SUCCESS && qid)
+    *qid = id;
+
+  return status;
+}
+
+void ares_query(ares_channel channel, const char *name, int dnsclass,
+                int type, ares_callback callback, void *arg)
+{
+  ares_query_qid(channel, name, dnsclass, type, callback, arg, NULL);
 }
 
+
 static void qcallback(void *arg, int status, int timeouts, unsigned char *abuf, int alen)
 {
   struct qquery *qquery = (struct qquery *) arg;

src/lib/ares_send.c

@@ -28,8 +28,8 @@
 #include "ares_dns.h"
 #include "ares_private.h"
 
-void ares_send(ares_channel channel, const unsigned char *qbuf, int qlen,
-               ares_callback callback, void *arg)
+int ares_send_ex(ares_channel channel, const unsigned char *qbuf, int qlen,
+                 ares_callback callback, void *arg)
 {
   struct query *query;
   int i, packetsz;
@@ -39,27 +39,28 @@ void ares_send(ares_channel channel, const unsigned char *qbuf, int qlen,
   if (qlen < HFIXEDSZ || qlen >= (1 << 16))
     {
       callback(arg, ARES_EBADQUERY, 0, NULL, 0);
-      return;
+      return ARES_EBADQUERY;
     }
   if (channel->nservers < 1)
     {
       callback(arg, ARES_ESERVFAIL, 0, NULL, 0);
-      return;
+      return ARES_ESERVFAIL;
     }
   /* Allocate space for query and allocated fields. */
   query = ares_malloc(sizeof(struct query));
   if (!query)
     {
       callback(arg, ARES_ENOMEM, 0, NULL, 0);
-      return;
+      return ARES_ENOMEM;
     }
+  memset(query, 0, sizeof(*query));
   query->channel = channel;
   query->tcpbuf = ares_malloc(qlen + 2);
   if (!query->tcpbuf)
     {
       ares_free(query);
       callback(arg, ARES_ENOMEM, 0, NULL, 0);
-      return;
+      return ARES_ENOMEM;
     }
   query->server_info = ares_malloc(channel->nservers *
                                    sizeof(query->server_info[0]));
@@ -68,7 +69,7 @@ void ares_send(ares_channel channel, const unsigned char *qbuf, int qlen,
       ares_free(query->tcpbuf);
       ares_free(query);
       callback(arg, ARES_ENOMEM, 0, NULL, 0);
-      return;
+      return ARES_ENOMEM;
     }
 
   /* Compute the query ID.  Start with no timeout. */
@@ -120,7 +121,7 @@ void ares_send(ares_channel channel, const unsigned char *qbuf, int qlen,
   if (query->node_all_queries == NULL) {
     callback(arg, ARES_ENOMEM, 0, NULL, 0);
     ares__free_query(query);
-    return;
+    return ARES_ENOMEM;
   }
 
   /* Keep track of queries bucketed by qid, so we can process DNS
@@ -129,10 +130,16 @@ void ares_send(ares_channel channel, const unsigned char *qbuf, int qlen,
   if (!ares__htable_stvp_insert(channel->queries_by_qid, query->qid, query)) {
     callback(arg, ARES_ENOMEM, 0, NULL, 0);
     ares__free_query(query);
-    return;
+    return ARES_ENOMEM;
   }
 
   /* Perform the first query action. */
   now = ares__tvnow();
-  ares__send_query(channel, query, &now);
+  return ares__send_query(channel, query, &now);
+}
+
+void ares_send(ares_channel channel, const unsigned char *qbuf, int qlen,
+               ares_callback callback, void *arg)
+{
+  ares_send_ex(channel, qbuf, qlen, callback, arg);
 }

test/ares-test-mock.cc

@@ -729,6 +729,29 @@ TEST_P(MockChannelTest, SearchHighNdots) {
             ss.str());
 }
 
+TEST_P(MockUDPChannelTest, V4WorksV6Timeout) {
+  std::vector<byte> nothing;
+  DNSPacket reply;
+  reply.set_response().set_aa()
+    .add_question(new DNSQuestion("www.google.com", T_A))
+    .add_answer(new DNSARR("www.google.com", 0x0100, {0x01, 0x02, 0x03, 0x04}));
+
+  ON_CALL(server_, OnRequest("www.google.com", T_A))
+    .WillByDefault(SetReply(&server_, &reply));
+
+  ON_CALL(server_, OnRequest("www.google.com", T_AAAA))
+    .WillByDefault(SetReplyData(&server_, nothing));
+
+  HostResult result;
+  ares_gethostbyname(channel_, "www.google.com.", AF_UNSPEC, HostCallback, &result);
+  Process();
+  EXPECT_TRUE(result.done_);
+  EXPECT_EQ(1, result.timeouts_);
+  std::stringstream ss;
+  ss << result.host_;
+  EXPECT_EQ("{'www.google.com' aliases=[] addrs=[1.2.3.4]}", ss.str());
+}
+
 TEST_P(MockChannelTest, UnspecifiedFamilyV6) {
   DNSPacket rsp6;
   rsp6.set_response().set_aa()