diff --git a/CMakeLists.txt b/CMakeLists.txt index c525b31..c067c79 100644 --- a/CMakeLists.txt +++ b/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 diff --git a/src/dns_poller.c b/src/dns_poller.c index b594288..19be684 100644 --- a/src/dns_poller.c +++ b/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) { diff --git a/src/doh_proxy.c b/src/doh_proxy.c index 3b485eb..e148e3e 100644 --- a/src/doh_proxy.c +++ b/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; diff --git a/tests/unit/test_dns_poller.c b/tests/unit/test_dns_poller.c index b28b523..9e9f75c 100644 --- a/tests/unit/test_dns_poller.c +++ b/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(); } diff --git a/tests/unit/test_resolver_update.c b/tests/unit/test_resolver_update.c new file mode 100644 index 0000000..83d5d71 --- /dev/null +++ b/tests/unit/test_resolver_update.c @@ -0,0 +1,122 @@ +#include +#include + +#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(); +}