ares_set_servers_*() should allow an empty server list

For historic reasons, we have users depending on ares_set_servers_*()
to return ARES_SUCCESS when passing no servers and actually *clear*
the server list.  It appears they do this for test cases to simulate
DNS unavailable or similar.  Presumably they could achieve the same
effect in other ways (point to localhost on a port that isn't in use).
But it seems like this might be wide-spread enough to cause headaches
so we just will document and test for this behavior, clearly it hasn't
caused "issues" for anyone with the old behavior.

See: nodejs/node#50800

Fix By: Brad House (@bradh352)

Author: bradh352

docs/ares_set_servers.3

@@ -42,7 +42,7 @@ The name server linked list pointer argument may be the result of a previous
 call to \fBares_get_servers(3)\fP or a linked list of \fBares_addr_node\fP structs
 set up by other means.
 .PP
-The \fBares_set_servers(3)\fP function also allows the specification of UDP and
+The \fBares_set_servers_ports(3)\fP function also allows the specification of UDP and
 TCP ports to be used for communication on a per-server basis.  The provided
 linked list argument may be the result of a previous call to
 \fBares_get_servers_ports(3)\fP or a linked list of \fBares_addr_port_node\fP structs
@@ -51,7 +51,9 @@ set up by other means.
 This function replaces any potentially previously configured name servers
 with the ones given in the linked list. So, in order to configure a channel
 with more than one name server all the desired ones must be specified in a
-single list.
+single list. Though not recommended, passing NULL will clear all configured
+servers and make an inoperable channel, this may be advantageous for test
+simulation but unlikely to be useful in production.
 .PP
 The function does not take ownership of the linked list argument.
 The caller is responsible for freeing the linked list when no longer needed.

docs/ares_set_servers_csv.3

@@ -30,7 +30,13 @@ int ares_set_servers_ports_csv(ares_channel_t *\fIchannel\fP, const char* \fIser
 The \fBares_set_servers_csv\fP and \fBares_set_servers_ports_csv\fPfunctions set
 the list of DNS servers that ARES will query.  As of v1.22.0 this function can
 be called on an active channel with running queries, previously it would return
-ARES_ENOTIMP. The format of the servers option is:
+ARES_ENOTIMP.
+
+Though not recommended, passing NULL for servers will clear all configured
+servers and make an inoperable channel, this may be advantageous for test
+simulation but unlikely to be useful in production.
+
+The format of the servers option is:
 
 host[:port][,host[:port]]...
 

src/lib/ares_options.c

@@ -234,12 +234,12 @@ static ares_status_t ares__init_options_servers(ares_channel_t       *channel,
                                                 const struct in_addr *servers,
                                                 size_t                nservers)
 {
-  ares__llist_t *slist;
+  ares__llist_t *slist = NULL;
   ares_status_t  status;
 
-  slist = ares_in_addr_to_server_config_llist(servers, nservers);
-  if (slist == NULL) {
-    return ARES_ENOMEM;
+  status = ares_in_addr_to_server_config_llist(servers, nservers, &slist);
+  if (status != ARES_SUCCESS) {
+    return status;
   }
 
   status = ares__servers_update(channel, slist, ARES_TRUE);

src/lib/ares_private.h

@@ -477,9 +477,10 @@ ares_status_t ares__sconfig_append(ares__llist_t         **sconfig,
                                    unsigned short          tcp_port);
 ares_status_t ares__sconfig_append_fromstr(ares__llist_t **sconfig,
                                            const char     *str);
-ares__llist_t *
+ares_status_t
   ares_in_addr_to_server_config_llist(const struct in_addr *servers,
-                                      size_t                nservers);
+                                      size_t                nservers,
+                                      ares__llist_t       **llist);
 
 struct ares_hosts_entry;
 typedef struct ares_hosts_entry ares_hosts_entry_t;

src/lib/ares_process.c

@@ -777,7 +777,7 @@ static struct server_state *ares__random_server(ares_channel_t *channel)
 
   /* Silence coverity, not possible */
   if (num_servers == 0)
-    num_servers = 1;
+    return NULL;
 
   ares__rand_bytes(channel->rand_state, &c, 1);
 
@@ -816,9 +816,8 @@ static size_t ares__calc_query_timeout(const struct query *query)
   size_t                rounds;
   size_t                num_servers = ares__slist_len(channel->servers);
 
-  /* Silence coverity, not possible */
   if (num_servers == 0)
-    num_servers = 1;
+    return 0;
 
   /* For each trip through the entire server list, we want to double the
    * retry from the last retry */
@@ -878,6 +877,11 @@ ares_status_t ares__send_query(struct query *query, struct timeval *now)
     server = ares__slist_first_val(channel->servers);
   }
 
+  if (server == NULL) {
+    end_query(channel, query, ARES_ESERVFAIL /* ? */, NULL, 0);
+    return ARES_ECONNREFUSED;
+  }
+
   if (query->using_tcp) {
     size_t prior_len = 0;
     /* Make sure the TCP socket for this server is set up and queue

src/lib/ares_update_servers.c

@@ -531,11 +531,14 @@ ares_status_t ares__servers_update(ares_channel_t *channel,
   size_t              idx = 0;
   ares_status_t       status;
 
-  if (channel == NULL || server_list == NULL ||
-      ares__llist_len(server_list) == 0) {
+  if (channel == NULL) {
     return ARES_EFORMERR;
   }
 
+  /* NOTE: a NULL or zero entry server list is considered valid due to
+   *       real-world people needing support for this for their test harnesses
+   */
+
   /* Add new entries */
   for (node = ares__llist_node_first(server_list); node != NULL;
        node = ares__llist_node_next(node)) {
@@ -589,19 +592,18 @@ ares_status_t ares__servers_update(ares_channel_t *channel,
   return status;
 }
 
-static ares__llist_t *
-  ares_addr_node_to_server_config_llist(const struct ares_addr_node *servers)
+static ares_status_t
+  ares_addr_node_to_server_config_llist(const struct ares_addr_node *servers,
+                                        ares__llist_t              **llist)
 {
   const struct ares_addr_node *node;
   ares__llist_t               *s;
 
-  if (servers == NULL) {
-    return NULL;
-  }
+  *llist = NULL;
 
   s = ares__llist_create(ares_free);
   if (s == NULL) {
-    return NULL;
+    goto fail;
   }
 
   for (node = servers; node != NULL; node = node->next) {
@@ -632,26 +634,26 @@ static ares__llist_t *
     }
   }
 
-  return s;
+  *llist = s;
+  return ARES_SUCCESS;
 
 fail:
   ares__llist_destroy(s);
-  return NULL;
+  return ARES_ENOMEM;
 }
 
-static ares__llist_t *ares_addr_port_node_to_server_config_llist(
-  const struct ares_addr_port_node *servers)
+static ares_status_t ares_addr_port_node_to_server_config_llist(
+  const struct ares_addr_port_node *servers,
+  ares__llist_t **llist)
 {
   const struct ares_addr_port_node *node;
   ares__llist_t                    *s;
 
-  if (servers == NULL) {
-    return NULL;
-  }
+  *llist = NULL;
 
   s = ares__llist_create(ares_free);
   if (s == NULL) {
-    return NULL;
+    goto fail;
   }
 
   for (node = servers; node != NULL; node = node->next) {
@@ -685,30 +687,30 @@ static ares__llist_t *ares_addr_port_node_to_server_config_llist(
     }
   }
 
-  return s;
+  *llist = s;
+  return ARES_SUCCESS;
 
 fail:
   ares__llist_destroy(s);
-  return NULL;
+  return ARES_ENOMEM;
 }
 
-ares__llist_t *
+ares_status_t
   ares_in_addr_to_server_config_llist(const struct in_addr *servers,
-                                      size_t                nservers)
+                                      size_t                nservers,
+                                      ares__llist_t       **llist)
 {
   size_t         i;
   ares__llist_t *s;
 
-  if (servers == NULL || nservers == 0) {
-    return NULL;
-  }
+  *llist = NULL;
 
   s = ares__llist_create(ares_free);
   if (s == NULL) {
-    return NULL;
+    goto fail;
   }
 
-  for (i = 0; i < nservers; i++) {
+  for (i = 0; servers != NULL && i < nservers; i++) {
     ares_sconfig_t *sconfig;
 
     sconfig = ares_malloc_zero(sizeof(*sconfig));
@@ -725,11 +727,12 @@ ares__llist_t *
     }
   }
 
-  return s;
+  *llist = s;
+  return ARES_SUCCESS;
 
 fail:
   ares__llist_destroy(s);
-  return NULL;
+  return ARES_ENOMEM;
 }
 
 int ares_get_servers(ares_channel_t *channel, struct ares_addr_node **servers)
@@ -842,13 +845,13 @@ int ares_set_servers(ares_channel_t              *channel,
   ares__llist_t *slist;
   ares_status_t  status;
 
-  if (channel == NULL || servers == NULL) {
+  if (channel == NULL) {
     return ARES_ENODATA;
   }
 
-  slist = ares_addr_node_to_server_config_llist(servers);
-  if (slist == NULL) {
-    return ARES_ENOMEM;
+  status = ares_addr_node_to_server_config_llist(servers, &slist);
+  if (status != ARES_SUCCESS) {
+    return (int)status;
   }
 
   status = ares__servers_update(channel, slist, ARES_TRUE);
@@ -864,13 +867,13 @@ int ares_set_servers_ports(ares_channel_t                   *channel,
   ares__llist_t *slist;
   ares_status_t  status;
 
-  if (channel == NULL || servers == NULL) {
+  if (channel == NULL) {
     return ARES_ENODATA;
   }
 
-  slist = ares_addr_port_node_to_server_config_llist(servers);
-  if (slist == NULL) {
-    return ARES_ENOMEM;
+  status = ares_addr_port_node_to_server_config_llist(servers, &slist);
+  if (status != ARES_SUCCESS) {
+    return (int)status;
   }
 
   status = ares__servers_update(channel, slist, ARES_TRUE);
@@ -904,7 +907,8 @@ static ares_status_t set_servers_csv(ares_channel_t *channel, const char *_csv,
 
   i = ares_strlen(_csv);
   if (i == 0) {
-    return ARES_SUCCESS; /* blank all servers */
+    /* blank all servers */
+    return (ares_status_t)ares_set_servers_ports(channel, NULL);
   }
 
   csv = ares_malloc(i + 2);

test/ares-test-misc.cc

@@ -45,7 +45,21 @@ TEST_F(DefaultChannelTest, GetServersFailures) {
 }
 
 TEST_F(DefaultChannelTest, SetServers) {
-  EXPECT_EQ(ARES_ENODATA, ares_set_servers(channel_, nullptr));
+  /* NOTE: This test is because we have actual external users doing test case
+   *       simulation and removing all servers to generate various error
+   *       conditions in their own code.  It would make more sense to return
+   *       ARES_ENODATA, but due to historical users, we can't break them.
+   *       See: https://github.com/nodejs/node/pull/50800
+   */
+  EXPECT_EQ(ARES_SUCCESS, ares_set_servers(channel_, nullptr));
+  std::vector<std::string> expected_empty = { };
+  EXPECT_EQ(expected_empty, GetNameServers(channel_));
+  HostResult result;
+  ares_gethostbyname(channel_, "www.google.com.", AF_INET, HostCallback, &result);
+  Process();
+  EXPECT_TRUE(result.done_);
+  EXPECT_EQ(ARES_ESERVFAIL, result.status_);
+
 
   struct ares_addr_node server1;
   struct ares_addr_node server2;
@@ -63,7 +77,15 @@ TEST_F(DefaultChannelTest, SetServers) {
 }
 
 TEST_F(DefaultChannelTest, SetServersPorts) {
-  EXPECT_EQ(ARES_ENODATA, ares_set_servers_ports(channel_, nullptr));
+  /* NOTE: This test is because we have actual external users doing test case
+   *       simulation and removing all servers to generate various error
+   *       conditions in their own code.  It would make more sense to return
+   *       ARES_ENODATA, but due to historical users, we can't break them.
+   *       See: https://github.com/nodejs/node/pull/50800
+   */
+  EXPECT_EQ(ARES_SUCCESS, ares_set_servers_ports(channel_, nullptr));
+  std::vector<std::string> expected_empty = { };
+  EXPECT_EQ(expected_empty, GetNameServers(channel_));
 
   struct ares_addr_port_node server1;
   struct ares_addr_port_node server2;
@@ -91,6 +113,21 @@ TEST_F(DefaultChannelTest, SetServersCSV) {
   EXPECT_EQ(ARES_ENODATA, ares_set_servers_csv(nullptr, "1.2.3.4.5"));
   EXPECT_EQ(ARES_ENODATA, ares_set_servers_csv(nullptr, "1:2:3:4:5"));
 
+  /* NOTE: This test is because we have actual external users doing test case
+   *       simulation and removing all servers to generate various error
+   *       conditions in their own code.  It would make more sense to return
+   *       ARES_ENODATA, but due to historical users, we can't break them.
+   *       See: https://github.com/nodejs/node/pull/50800
+   */
+  EXPECT_EQ(ARES_SUCCESS, ares_set_servers_csv(channel_, NULL));
+  std::vector<std::string> expected_empty = { };
+  EXPECT_EQ(expected_empty, GetNameServers(channel_));
+  EXPECT_EQ(ARES_SUCCESS, ares_set_servers_csv(channel_, ""));
+  EXPECT_EQ(expected_empty, GetNameServers(channel_));
+
+
+
+
   EXPECT_EQ(ARES_SUCCESS,
             ares_set_servers_csv(channel_, "1.2.3.4,0102:0304:0506:0708:0910:1112:1314:1516,2.3.4.5"));
   std::vector<std::string> expected = {"1.2.3.4:53", "[0102:0304:0506:0708:0910:1112:1314:1516]:53", "2.3.4.5:53"};