Fix resolver list getting stuck after a partial bootstrap result

CMakeLists.txt

@@ -225,6 +225,17 @@ if(BUILD_TESTING)
   add_test(NAME dns_truncate COMMAND dns_truncate_test)
   set_tests_properties(dns_truncate PROPERTIES LABELS unit)
 
+  add_executable(resolver_update_test
+    tests/unit/test_resolver_update.c
+    src/logging.c
+    src/ring_buffer.c)
+  set_property(SOURCE tests/unit/test_resolver_update.c APPEND
+    PROPERTY COMPILE_DEFINITIONS __FILENAME__="resolver_update_test")
+  target_link_libraries(resolver_update_test curl ev)
+  set_property(TARGET resolver_update_test PROPERTY C_STANDARD 11)
+  add_test(NAME resolver_update COMMAND resolver_update_test)
+  set_tests_properties(resolver_update PROPERTIES LABELS unit)
+
   find_program(VALGRIND_EXE NAMES valgrind)
   if(VALGRIND_EXE)
     add_test(NAME dns_truncate_valgrind

src/dns_poller.c

@@ -234,6 +234,14 @@ void dns_poller_init(dns_poller_t *d, struct ev_loop *loop,
     .sock_state_cb_data = d
   };
   int optmask = ARES_OPT_TIMEOUTMS | ARES_OPT_TRIES | ARES_OPT_SOCK_STATE_CB;
+#ifdef ARES_OPT_QUERY_CACHE
+  // The poller re-resolves on its own schedule, so the c-ares query cache
+  // (on by default since c-ares 1.31) adds nothing. While the network is
+  // down it answers from whichever of the A/AAAA records has not expired
+  // yet, and that partial list would be installed as a change.
+  options.qcache_max_ttl = 0;
+  optmask |= ARES_OPT_QUERY_CACHE;
+#endif
 
   r = ares_init_options(&d->ares, &options, optmask);
   if (r != ARES_SUCCESS) {

src/doh_proxy.c

@@ -73,10 +73,26 @@ void doh_proxy_set_resolv(doh_proxy_t *p, const char *buf) {
   }
 }
 
-// Returns 1 if `addr_list` is a (possibly equal, possibly proper) subset of
-// `full_list`, where both are comma-separated IP literals. Used to decide
-// whether a fresh poll result actually changed anything; if every IP in the
-// new list is already in the old list, we skip the curl reset.
+// Returns 1 if the comma-separated `list` has an element that is exactly the
+// `address_len` bytes at `address`. Whole elements are compared, so
+// "192.0.2.1" does not match "192.0.2.10".
+static int addr_list_contains(const char *list, const char *address,
+                              size_t address_len) {
+  const char *pos = list;
+  const char *end = list + strlen(list);
+  while (pos < end) {
+    const char *comma = strchr(pos, ',');
+    const size_t element_len = (size_t)(comma ? comma - pos : end - pos);
+    if (element_len == address_len && strncmp(pos, address, address_len) == 0) {
+      return 1;
+    }
+    pos += element_len + 1;
+  }
+  return 0;
+}
+
+// Returns 1 if `list` contains an address that is missing from `full_list`.
+// Both are comma-separated IP literals.
 static int addr_list_reduced(const char* full_list, const char* list) {
   const char *pos = list;
   const char *end = list + strlen(list);
@@ -91,10 +107,7 @@ static int addr_list_reduced(const char* full_list, const char* list) {
     strncpy(current, pos, ip_len);
     current[ip_len] = '\0';
 
-    const char *match_begin = strstr(full_list, current);
-    if (!match_begin ||
-        !(match_begin == full_list || *(match_begin - 1) == ',') ||
-        !(*(match_begin + ip_len) == ',' || *(match_begin + ip_len) == '\0')) {
+    if (!addr_list_contains(full_list, current, ip_len)) {
       DLOG("IP address missing: %s", current);
       return 1;
     }
@@ -128,7 +141,12 @@ void doh_proxy_handle_resolver_update(const char *hostname, void *ctx,
     char * old_addr_list = strstr(p->resolv->data, port_colon);
     if (old_addr_list) {
       old_addr_list += strlen(port_colon);
-      if (!addr_list_reduced(addr_list, old_addr_list)) {
+      // Only an identical set of addresses counts as unchanged. A poll made
+      // while the network is down can return a subset (e.g. c-ares still had
+      // only the AAAA answer cached). That subset gets installed, so the
+      // full list that comes back afterwards must be installed again.
+      if (!addr_list_reduced(addr_list, old_addr_list) &&
+          !addr_list_reduced(old_addr_list, addr_list)) {
         DLOG("DNS server IP address unchanged (%s).", buf + ip_start);
         free((void*)addr_list);
         return;

tests/unit/test_dns_poller.c

@@ -126,10 +126,25 @@ static void test_cleanup_stops_active_watchers(void) {
   TEST_ASSERT(ev_run(loop, EVRUN_NOWAIT) == 0);
 }
 
+static void test_query_cache_disabled(void) {
+#ifdef ARES_OPT_QUERY_CACHE
+  struct ares_options saved;
+  int optmask = 0;
+  memset(&saved, 0, sizeof(saved));
+  TEST_ASSERT(ares_save_options(poller.ares, &saved, &optmask) == ARES_SUCCESS);
+  const int cache_option_set = (optmask & ARES_OPT_QUERY_CACHE) != 0;
+  const unsigned int max_ttl = saved.qcache_max_ttl;
+  ares_destroy_options(&saved);
+  TEST_ASSERT(cache_option_set);
+  TEST_ASSERT(max_ttl == 0);
+#endif
+}
+
 int main(void) {
   TEST_RUN(test_allocates_watchers_beyond_nameserver_count);
   TEST_RUN(test_reuses_released_watcher);
   TEST_RUN(test_fd_zero_does_not_alias_free_watcher);
   TEST_RUN(test_cleanup_stops_active_watchers);
+  TEST_RUN(test_query_cache_disabled);
   return test_summary();
 }

tests/unit/test_resolver_update.c

@@ -0,0 +1,122 @@
+#include <stdlib.h>
+#include <string.h>
+
+#include "test_harness.h"
+#include "../../src/doh_proxy.c"
+
+static https_client_t client;
+static doh_proxy_t *proxy;
+static unsigned reset_calls;
+
+void https_client_reset(https_client_t *unused_client) {
+  (void)unused_client;
+  reset_calls++;
+}
+
+void https_client_fetch(https_client_t *unused_client, const char *url,
+                        const char *postdata, size_t postdata_len,
+                        struct curl_slist *resolv, uint16_t id,
+                        https_response_cb cb, void *data) {
+  (void)unused_client;
+  (void)url;
+  (void)postdata;
+  (void)postdata_len;
+  (void)resolv;
+  (void)id;
+  (void)cb;
+  (void)data;
+}
+
+void stat_request_begin(stat_t *stat, size_t request_len, uint8_t is_tcp) {
+  (void)stat;
+  (void)request_len;
+  (void)is_tcp;
+}
+
+void stat_request_end(stat_t *stat, size_t response_len,
+                      ev_tstamp latency, uint8_t is_tcp) {
+  (void)stat;
+  (void)response_len;
+  (void)latency;
+  (void)is_tcp;
+}
+
+static void setUp(void) {
+  memset(&client, 0, sizeof(client));
+  reset_calls = 0;
+  proxy = doh_proxy_create(NULL, &client, "https://resolver.test/dns-query", NULL);
+}
+
+static void tearDown(void) {
+  doh_proxy_destroy(proxy);
+  proxy = NULL;
+}
+
+static void update_resolver(const char *addresses) {
+  char *owned_addresses = strdup(addresses);
+  if (owned_addresses == NULL) {
+    test_fail(__FILE__, __LINE__, "strdup(addresses)");
+    return;
+  }
+  doh_proxy_handle_resolver_update("resolver.test", proxy, owned_addresses);
+}
+
+static int resolv_is(const char *expected) {
+  return proxy->resolv != NULL && proxy->resolv->data != NULL &&
+         strcmp(proxy->resolv->data, expected) == 0;
+}
+
+// A poll made while the network was down returned only the AAAA records
+// (c-ares still had them cached, the A query timed out). When the network is
+// back the full list must be installed again; otherwise an IPv4-only host is
+// left with an IPv6-only list and every request fails.
+static void test_full_list_restored_after_partial_poll(void) {
+  update_resolver("192.0.2.1,192.0.2.2,2001:db8::1,2001:db8::2");
+  update_resolver("2001:db8::1,2001:db8::2");
+  TEST_ASSERT(resolv_is("resolver.test:443:2001:db8::1,2001:db8::2"));
+
+  update_resolver("192.0.2.1,192.0.2.2,2001:db8::1,2001:db8::2");
+  TEST_ASSERT(resolv_is(
+      "resolver.test:443:192.0.2.1,192.0.2.2,2001:db8::1,2001:db8::2"));
+  TEST_ASSERT(reset_calls == 3);
+}
+
+static void test_added_address_is_installed(void) {
+  update_resolver("192.0.2.1");
+  update_resolver("192.0.2.1,192.0.2.2");
+  TEST_ASSERT(resolv_is("resolver.test:443:192.0.2.1,192.0.2.2"));
+  TEST_ASSERT(reset_calls == 2);
+}
+
+static void test_removed_address_is_installed(void) {
+  update_resolver("192.0.2.1,192.0.2.2");
+  update_resolver("192.0.2.1");
+  TEST_ASSERT(resolv_is("resolver.test:443:192.0.2.1"));
+  TEST_ASSERT(reset_calls == 2);
+}
+
+static void test_reordered_list_is_unchanged(void) {
+  update_resolver("192.0.2.1,192.0.2.2");
+  update_resolver("192.0.2.2,192.0.2.1");
+  TEST_ASSERT(resolv_is("resolver.test:443:192.0.2.1,192.0.2.2"));
+  TEST_ASSERT(reset_calls == 1);
+}
+
+static void test_address_prefix_is_not_a_match(void) {
+  update_resolver("192.0.2.10,192.0.2.1");
+  update_resolver("192.0.2.1,192.0.2.10");
+  TEST_ASSERT(reset_calls == 1);
+
+  update_resolver("192.0.2.1");
+  TEST_ASSERT(resolv_is("resolver.test:443:192.0.2.1"));
+  TEST_ASSERT(reset_calls == 2);
+}
+
+int main(void) {
+  TEST_RUN(test_full_list_restored_after_partial_poll);
+  TEST_RUN(test_added_address_is_installed);
+  TEST_RUN(test_removed_address_is_installed);
+  TEST_RUN(test_reordered_list_is_unchanged);
+  TEST_RUN(test_address_prefix_is_not_a_match);
+  return test_summary();
+}