Windows DNS server sorting (#81)

Original Patch From Brad Spencer:
https://c-ares.haxx.se/mail/c-ares-archive-2016-04/0000.shtml

My modifications include:
 * Dynamically find GetBestRoute2 since it is a Windows Vista+ symbol, and will fall back to prior behavior when not available.
 * Prefer get_DNS_AdaptersAddresses as the modifications should alleviate the concerns which caused us to prefer get_DNS_NetworkParams
 * Update AppVeyor to use MinGW-w64 instead of the legacy MinGW
 * Fix compile error in test suite for Windows.

Original message from patch below:

From: Brad Spencer <bspencer@blackberry.com>
Date: Fri, 29 Apr 2016 14:26:23 -0300

On Windows, the c-ares DNS resolver tries first to get a full list of
DNS server addresses by enumerating the system's IPv4/v6 interfaces and
then getting the per-interface DNS server lists from those interfaces
and joining them together. The OS, at least in the way the c-ares
prefers to query them (which also may be the only or best way in some
environments), does not provide a unified list of DNS servers ordered
according to "current network conditions". Currently, c-ares will then
try to use them in whatever order the nested enumeration produces, which
may result in DNS requests being sent to servers on one interface
(hosting the current default route, for example) that are only intended
to be used via another interface (intended to be used when the first
interface is not available, for example). This, in turn, can lead to
spurious failures and timeouts simply because of the server address
order that resulted because of the enumeration process.

This patch makes the (safe?) assumption that there is no other better
rule to chose which interface's DNS server list should be prioritized.
After all, a DNS lookup isn't something "per network"; applications
don't look up "these DNS names on this interface and those DNS names on
that interface". There is a single resource pool of DNS servers and the
application should presume that any server will give it the "right"
answer. However, even if all DNS servers are assumed to give equally
useful responses, it is reasonable to expect that some DNS servers will
not accept requests on all interfaces. This patch avoids the problem by
sorting the DNS server addresses using the Windows IPv4/v6 routing tables.

For example, a request to DNS server C on interface 2 that is actually
sent over interface 1 (which may happen to have the default route) may
be rejected by or not delivered to DNS server C. So, better to use DNS
servers A and B associated with interface 1, at least as a first try.

By using the metric of the route to the DNS server itself as a proxy for
priority of the DNS server in the list, this patch is able to adapt
dynamically to changes in the interface list, the DNS server lists per
interface, which interfaces are active, the routing table, and so on,
while always picking a good "best" DNS server first.

In cases where any DNS server on any interface will do, this patch still
seems useful because it will prioritize a lower-metric route's (and thus
interface's) servers.

CMakeLists.txt

@@ -166,7 +166,7 @@ ELSEIF (CMAKE_SYSTEM_NAME STREQUAL "AIX")
 ELSEIF (CMAKE_SYSTEM_NAME STREQUAL "FreeBSD")
 	# Don't define _XOPEN_SOURCE on FreeBSD, it actually reduces visibility instead of increasing it
 ELSEIF (WIN32)
-	LIST (APPEND SYSFLAGS -DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_DEPRECATE -D_CRT_NONSTDC_NO_DEPRECATE)
+	LIST (APPEND SYSFLAGS -DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_DEPRECATE -D_CRT_NONSTDC_NO_DEPRECATE -D_WIN32_WINNT=0x0600)
 ENDIF ()
 ADD_DEFINITIONS(${SYSFLAGS})
 

Makefile.Watcom

@@ -52,7 +52,7 @@ LFLAGS += debug all
 CFLAGS += -d0
 !endif
 
-CFLAGS += -d_WIN32_WINNT=0x0501
+CFLAGS += -d_WIN32_WINNT=0x0600
 
 #
 # Change to suite.

Makefile.m32

@@ -17,7 +17,7 @@ RANLIB	= $(CROSSPREFIX)ranlib
 #RM	= rm -f
 CP	= cp -afv
 
-CFLAGS	= $(CARES_CFLAG_EXTRAS) -O2 -Wall -I.
+CFLAGS	= $(CARES_CFLAG_EXTRAS) -O2 -Wall -I. -D_WIN32_WINNT=0x0600
 CFLAGS	+= -DCARES_STATICLIB
 LDFLAGS	= $(CARES_LDFLAG_EXTRAS) -s
 LIBS	= -lwsock32

Makefile.msvc

@@ -184,7 +184,7 @@ CFLAGS  = /UWIN32 /DWATT32 /I$(WATT_ROOT)\inc
 EX_LIBS_REL = $(WATT_ROOT)\lib\wattcpvc_imp.lib
 EX_LIBS_DBG = $(WATT_ROOT)\lib\wattcpvc_imp_d.lib
 !ELSE
-CFLAGS  = /DWIN32
+CFLAGS  = /DWIN32 /D_WIN32_WINNT=0x0600
 EX_LIBS_REL = ws2_32.lib advapi32.lib kernel32.lib
 EX_LIBS_DBG = ws2_32.lib advapi32.lib kernel32.lib
 !ENDIF

appveyor.yml

@@ -6,8 +6,8 @@ build_script:
   - if "%platform%" == "x86" call "%VS120COMNTOOLS%\..\..\VC\vcvarsall.bat"
   - if "%platform%" == "x64" "C:\Program Files\Microsoft SDKs\Windows\v7.1\Bin\SetEnv.cmd" /x64
   - if "%platform%" == "x64" call "%VS120COMNTOOLS%\..\..\VC\vcvarsall.bat" x86_amd64
-  - if "%platform%" == "mingw" set PATH=%PATH%;C:\MinGW\bin;C:\MinGW\MSYS\1.0\local\bin;C:\MinGW\MSYS\1.0\bin
+  - if "%platform%" == "mingw" set PATH=%PATH%;C:\mingw-w64\i686-5.3.0-posix-dwarf-rt_v4-rev0\mingw32\bin
   - copy ares_build.h.dist ares_build.h
-  - if "%platform%" == "mingw" ( make -f Makefile.m32 demos ) else ( nmake /f Makefile.msvc )
+  - if "%platform%" == "mingw" ( mingw32-make.exe -f Makefile.m32 demos ) else ( nmake /f Makefile.msvc )
   - cd test
-  - if "%platform%" == "mingw" ( make -f Makefile.m32 ) else ( nmake /f Makefile.msvc vtest )
+  - if "%platform%" == "mingw" ( mingw32-make.exe -f Makefile.m32 ) else ( nmake /f Makefile.msvc vtest )

ares_init.c

@@ -943,6 +943,116 @@ done:
   return 1;
 }
 
+static BOOL ares_IsWindowsVistaOrGreater(void)
+{
+  OSVERSIONINFO vinfo;
+  memset(&vinfo, 0, sizeof(vinfo));
+  vinfo.dwOSVersionInfoSize = sizeof(vinfo);
+  if (!GetVersionEx(&vinfo) || vinfo.dwMajorVersion < 6)
+    return FALSE;
+  return TRUE;
+}
+
+/* A structure to hold the string form of IPv4 and IPv6 addresses so we can
+ * sort them by a metric.
+ */
+typedef struct
+{
+  /* The metric we sort them by. */
+  ULONG metric;
+
+  /* Room enough for the string form of any IPv4 or IPv6 address that
+   * ares_inet_ntop() will create.  Based on the existing c-ares practice.
+   */
+  char text[sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255")];
+} Address;
+
+/* Sort Address values \a left and \a right by metric, returning the usual
+ * indicators for qsort().
+ */
+static int compareAddresses(const void *arg1,
+                            const void *arg2)
+{
+  const Address * const left = arg1;
+  const Address * const right = arg2;
+  if(left->metric < right->metric) return -1;
+  if(left->metric > right->metric) return 1;
+  return 0;
+}
+
+/* There can be multiple routes to "the Internet".  And there can be different
+ * DNS servers associated with each of the interfaces that offer those routes.
+ * We have to assume that any DNS server can serve any request.  But, some DNS
+ * servers may only respond if requested over their associated interface.  But
+ * we also want to use "the preferred route to the Internet" whenever possible
+ * (and not use DNS servers on a non-preferred route even by forcing request
+ * to go out on the associated non-preferred interface).  i.e. We want to use
+ * the DNS servers associated with the same interface that we would use to
+ * make a general request to anything else.
+ *
+ * But, Windows won't sort the DNS servers by the metrics associated with the
+ * routes and interfaces _even_ though it obviously sends IP packets based on
+ * those same routes and metrics.  So, we must do it ourselves.
+ *
+ * So, we sort the DNS servers by the same metric values used to determine how
+ * an outgoing IP packet will go, thus effectively using the DNS servers
+ * associated with the interface that the DNS requests themselves will
+ * travel.  This gives us optimal routing and avoids issues where DNS servers
+ * won't respond to requests that don't arrive via some specific subnetwork
+ * (and thus some specific interface).
+ *
+ * This function computes the metric we use to sort.  On the interface
+ * identified by \a luid, it determines the best route to \a dest and combines
+ * that route's metric with \a interfaceMetric to compute a metric for the
+ * destination address on that interface.  This metric can be used as a weight
+ * to sort the DNS server addresses associated with each interface (lower is
+ * better).
+ *
+ * Note that by restricting the route search to the specific interface with
+ * which the DNS servers are associated, this function asks the question "What
+ * is the metric for sending IP packets to this DNS server?" which allows us
+ * to sort the DNS servers correctly.
+ */
+static ULONG getBestRouteMetric(IF_LUID * const luid, /* Can't be const :( */
+                                const SOCKADDR_INET * const dest,
+                                const ULONG interfaceMetric)
+{
+  /* On this interface, get the best route to that destination. */
+  MIB_IPFORWARD_ROW2 row;
+  SOCKADDR_INET ignored;
+  if(!ares_fpGetBestRoute2 ||
+     ares_fpGetBestRoute2(/* The interface to use.  The index is ignored since we are
+                           * passing a LUID.
+                           */
+                           luid, 0,
+                           /* No specific source address. */
+                           NULL,
+                           /* Our destination address. */
+                           dest,
+                           /* No options. */
+                           0,
+                           /* The route row. */
+                           &row,
+                           /* The best source address, which we don't need. */
+                           &ignored) != NO_ERROR
+     /* If the metric is "unused" (-1) or too large for us to add the two
+      * metrics, use the worst possible, thus sorting this last.
+      */
+     || row.Metric == (ULONG)-1
+     || row.Metric > ((ULONG)-1) - interfaceMetric) {
+    /* Return the worst possible metric. */
+    return (ULONG)-1;
+  }
+
+  /* Return the metric value from that row, plus the interface metric.
+   *
+   * See
+   * http://msdn.microsoft.com/en-us/library/windows/desktop/aa814494(v=vs.85).aspx
+   * which describes the combination as a "sum".
+   */
+  return row.Metric + interfaceMetric;
+}
+
 /*
  * get_DNS_AdaptersAddresses()
  *
@@ -969,14 +1079,19 @@ static int get_DNS_AdaptersAddresses(char **outptr)
   int trying = IPAA_MAX_TRIES;
   int res;
 
+  /* The capacity of addresses, in elements. */
+  size_t addressesSize;
+  /* The number of elements in addresses. */
+  size_t addressesIndex = 0;
+  /* The addresses we will sort. */
+  Address *addresses;
+
   union {
     struct sockaddr     *sa;
     struct sockaddr_in  *sa4;
     struct sockaddr_in6 *sa6;
   } namesrvr;
 
-  char txtaddr[sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255")];
-
   *outptr = NULL;
 
   /* Verify run-time availability of GetAdaptersAddresses() */
@@ -987,6 +1102,17 @@ static int get_DNS_AdaptersAddresses(char **outptr)
   if (!ipaa)
     return 0;
 
+  /* Start with enough room for a few DNS server addresses and we'll grow it
+   * as we encounter more.
+   */
+  addressesSize = 4;
+  addresses = (Address*)ares_malloc(sizeof(Address) * addressesSize);
+  if(addresses == NULL) {
+    /* We need room for at least some addresses to function. */
+    ares_free(ipaa);
+    return 0;
+  }
+
   /* Usually this call suceeds with initial buffer size */
   res = (*ares_fpGetAdaptersAddresses) (AF_UNSPEC, AddrFlags, NULL,
                                         ipaa, &ReqBufsz);
@@ -1016,6 +1142,12 @@ static int get_DNS_AdaptersAddresses(char **outptr)
     if(ipaaEntry->OperStatus != IfOperStatusUp)
         continue;
 
+    /* For each interface, find any associated DNS servers as IPv4 or IPv6
+     * addresses.  For each found address, find the best route to that DNS
+     * server address _on_ _that_ _interface_ (at this moment in time) and
+     * compute the resulting total metric, just as Windows routing will do.
+     * Then, sort all the addresses found by the metric.
+     */
     for (ipaDNSAddr = ipaaEntry->FirstDnsServerAddress;
          ipaDNSAddr;
          ipaDNSAddr = ipaDNSAddr->Next)
@@ -1027,35 +1159,117 @@ static int get_DNS_AdaptersAddresses(char **outptr)
         if ((namesrvr.sa4->sin_addr.S_un.S_addr == INADDR_ANY) ||
             (namesrvr.sa4->sin_addr.S_un.S_addr == INADDR_NONE))
           continue;
+
+        /* Allocate room for another address, if necessary, else skip. */
+        if(addressesIndex == addressesSize) {
+          const size_t newSize = addressesSize + 4;
+          Address * const newMem = 
+            (Address*)ares_realloc(addresses, sizeof(Address) * newSize);
+          if(newMem == NULL) {
+            continue;
+          }
+          addresses = newMem;
+          addressesSize = newSize;
+        }
+
+        /* Vista required for Luid or Ipv4Metric */
+        if (ares_IsWindowsVistaOrGreater())
+        {
+          /* Save the address as the next element in addresses. */
+          addresses[addressesIndex].metric =
+            getBestRouteMetric(&ipaaEntry->Luid,
+                               (SOCKADDR_INET*)(namesrvr.sa),
+                               ipaaEntry->Ipv4Metric);
+        }
+        else
+        {
+          addresses[addressesIndex].metric = -1;
+        }
+
         if (! ares_inet_ntop(AF_INET, &namesrvr.sa4->sin_addr,
-                             txtaddr, sizeof(txtaddr)))
+                             addresses[addressesIndex].text,
+                             sizeof(addresses[0].text))) {
           continue;
+        }
+        ++addressesIndex;
       }
       else if (namesrvr.sa->sa_family == AF_INET6)
       {
         if (memcmp(&namesrvr.sa6->sin6_addr, &ares_in6addr_any,
                    sizeof(namesrvr.sa6->sin6_addr)) == 0)
           continue;
+
+        /* Allocate room for another address, if necessary, else skip. */
+        if(addressesIndex == addressesSize) {
+          const size_t newSize = addressesSize + 4;
+          Address * const newMem =
+            (Address*)ares_realloc(addresses, sizeof(Address) * newSize);
+          if(newMem == NULL) {
+            continue;
+          }
+          addresses = newMem;
+          addressesSize = newSize;
+        }
+
+        /* Vista required for Luid or Ipv4Metric */
+        if (ares_IsWindowsVistaOrGreater())
+        {
+          /* Save the address as the next element in addresses. */
+          addresses[addressesIndex].metric =
+            getBestRouteMetric(&ipaaEntry->Luid,
+                               (SOCKADDR_INET*)(namesrvr.sa), 
+                               ipaaEntry->Ipv6Metric);
+        }
+        else
+        {
+          addresses[addressesIndex].metric = -1;
+        }
+
         if (! ares_inet_ntop(AF_INET6, &namesrvr.sa6->sin6_addr,
-                             txtaddr, sizeof(txtaddr)))
+                             addresses[addressesIndex].text,
+                             sizeof(addresses[0].text))) {
           continue;
+        }
+        ++addressesIndex;
       }
-      else
+      else {
+        /* Skip non-IPv4/IPv6 addresses completely. */
         continue;
+      }
+    }
+  }
 
-      commajoin(outptr, txtaddr);
+  /* Sort all of the textual addresses by their metric. */
+  qsort(addresses, addressesIndex, sizeof(*addresses), compareAddresses);
 
-      if (!*outptr)
-        goto done;
+  /* Join them all into a single string, removing duplicates. */
+  {
+    size_t i;
+    for(i = 0; i < addressesIndex; ++i) {
+      size_t j;
+      /* Look for this address text appearing previously in the results. */
+      for(j = 0; j < i; ++j) {
+        if(strcmp(addresses[j].text, addresses[i].text) == 0) {
+          break;
+        }
+      }
+      /* Iff we didn't emit this address already, emit it now. */
+      if(j == i) {
+        /* Add that to outptr (if we can). */
+        commajoin(outptr, addresses[i].text);
+      }
     }
   }
 
 done:
+  ares_free(addresses);
+  
   if (ipaa)
     ares_free(ipaa);
 
-  if (!*outptr)
+  if (!*outptr) {
     return 0;
+  }
 
   return 1;
 }
@@ -1076,18 +1290,13 @@ done:
  */
 static int get_DNS_Windows(char **outptr)
 {
-  /*
-     Use GetNetworkParams First in case of
-     multiple adapter is enabled on this machine.
-     GetAdaptersAddresses will retrive dummy dns servers.
-     That will slowing DNS lookup.
-  */
-  /* Try using IP helper API GetNetworkParams() */
-  if (get_DNS_NetworkParams(outptr))
+  /* Try using IP helper API GetAdaptersAddresses(). IPv4 + IPv6, also sorts
+   * DNS servers by interface route metrics to try to use the best DNS server. */
+  if (get_DNS_AdaptersAddresses(outptr))
     return 1;
 
-  /* Try using IP helper API GetAdaptersAddresses() */
-  if (get_DNS_AdaptersAddresses(outptr))
+  /* Try using IP helper API GetNetworkParams(). IPv4 only. */
+  if (get_DNS_NetworkParams(outptr))
     return 1;
 
   /* Fall-back to registry information */

ares_library_init.c

@@ -27,6 +27,7 @@
 fpGetNetworkParams_t ares_fpGetNetworkParams = ZERO_NULL;
 fpSystemFunction036_t ares_fpSystemFunction036 = ZERO_NULL;
 fpGetAdaptersAddresses_t ares_fpGetAdaptersAddresses = ZERO_NULL;
+fpGetBestRoute2_t ares_fpGetBestRoute2 = ZERO_NULL;
 #endif
 
 /* library-private global vars with source visibility restricted to this file */
@@ -71,6 +72,15 @@ static int ares_win32_init(void)
          support Windows 2000 anymore */
     }
 
+  ares_fpGetBestRoute2 = (fpGetBestRoute2_t)
+    GetProcAddress(hnd_iphlpapi, "GetBestRoute2");
+  if (!ares_fpGetBestRoute2)
+    {
+      /* This can happen on clients before Vista, I don't
+         think it should be an error, unless we don't want to
+         support Windows XP anymore */
+    }
+
   /*
    * When advapi32.dll is unavailable or advapi32.dll has no SystemFunction036,
    * also known as RtlGenRandom, which is the case for Windows versions prior

ares_library_init.h

@@ -28,13 +28,14 @@
 typedef DWORD (WINAPI *fpGetNetworkParams_t) (FIXED_INFO*, DWORD*);
 typedef BOOLEAN (APIENTRY *fpSystemFunction036_t) (void*, ULONG);
 typedef ULONG (WINAPI *fpGetAdaptersAddresses_t) ( ULONG, ULONG, void*, IP_ADAPTER_ADDRESSES*, ULONG* );
-
+typedef NETIO_STATUS (WINAPI *fpGetBestRoute2_t) ( NET_LUID *, NET_IFINDEX, const SOCKADDR_INET *, const SOCKADDR_INET *, ULONG, PMIB_IPFORWARD_ROW2, SOCKADDR_INET * );
 /* Forward-declaration of variables defined in ares_library_init.c */
 /* that are global and unique instances for whole c-ares library.  */
 
 extern fpGetNetworkParams_t ares_fpGetNetworkParams;
 extern fpSystemFunction036_t ares_fpSystemFunction036;
 extern fpGetAdaptersAddresses_t ares_fpGetAdaptersAddresses;
+extern fpGetBestRoute2_t ares_fpGetBestRoute2;
 
 #endif /* USE_WINSOCK */
 

config-win32.h

@@ -259,31 +259,19 @@
 #  define _CRT_NONSTDC_NO_DEPRECATE 1
 #endif
 
-/* Officially, Microsoft's Windows SDK versions 6.X do not support Windows
-   2000 as a supported build target. VS2008 default installations provide
-   an embedded Windows SDK v6.0A along with the claim that Windows 2000 is
-   a valid build target for VS2008. Popular belief is that binaries built
-   with VS2008 using Windows SDK versions 6.X and Windows 2000 as a build
-   target are functional. */
+/* Set the Target to Vista. However, any symbols required above Win2000 
+ * should be loaded via LoadLibrary() */
 #if defined(_MSC_VER) && (_MSC_VER >= 1500)
-#  define VS2008_MIN_TARGET 0x0500
-#endif
-
-/* When no build target is specified VS2008 default build target is Windows
-   Vista, which leaves out even Winsows XP. If no build target has been given
-   for VS2008 we will target the minimum Officially supported build target,
-   which happens to be Windows XP. */
-#if defined(_MSC_VER) && (_MSC_VER >= 1500)
-#  define VS2008_DEF_TARGET  0x0501
+#  define VS2008_MIN_TARGET 0x0600
 #endif
 
 /* VS2008 default target settings and minimum build target check. */
 #if defined(_MSC_VER) && (_MSC_VER >= 1500)
 #  ifndef _WIN32_WINNT
-#    define _WIN32_WINNT VS2008_DEF_TARGET
+#    define _WIN32_WINNT VS2008_MIN_TARGET
 #  endif
 #  ifndef WINVER
-#    define WINVER VS2008_DEF_TARGET
+#    define WINVER VS2008_MIN_TARGET
 #  endif
 #  if (_WIN32_WINNT < VS2008_MIN_TARGET) || (WINVER < VS2008_MIN_TARGET)
 #    error VS2008 does not support Windows build targets prior to Windows 2000
@@ -291,13 +279,13 @@
 #endif
 
 /* When no build target is specified Pelles C 5.00 and later default build
-   target is Windows Vista. We override default target to be Windows 2000. */
+   target is Windows Vista. */
 #if defined(__POCC__) && (__POCC__ >= 500)
 #  ifndef _WIN32_WINNT
-#    define _WIN32_WINNT 0x0500
+#    define _WIN32_WINNT 0x0600
 #  endif
 #  ifndef WINVER
-#    define WINVER 0x0500
+#    define WINVER 0x0600
 #  endif
 #endif
 

configure.ac

@@ -194,6 +194,7 @@ case X-"$ac_cv_native_windows" in
     CURL_CHECK_HEADER_WINSOCK
     CURL_CHECK_HEADER_WINSOCK2
     CURL_CHECK_HEADER_WS2TCPIP
+    CPPFLAGS="$CPPFLAGS -D_WIN32_WINNT=0x0600"
     ;;
   *)
     ac_cv_header_winsock_h="no"

test/ares-test-live.cc

@@ -545,7 +545,7 @@ VIRT_NONVIRT_TEST_F(DefaultChannelTest, LiveGetNameInfoAllocFail) {
 }
 
 VIRT_NONVIRT_TEST_F(DefaultChannelTest, GetSock) {
-  ares_socket_t socks[3] = {-1, -1, -1};
+  ares_socket_t socks[3] = {ARES_SOCKET_BAD, ARES_SOCKET_BAD, ARES_SOCKET_BAD};
   int bitmask = ares_getsock(channel_, socks, 3);
   EXPECT_EQ(0, bitmask);
   bitmask = ares_getsock(channel_, nullptr, 0);
@@ -571,7 +571,7 @@ TEST_F(LibraryTest, GetTCPSock) {
   EXPECT_EQ(ARES_SUCCESS, ares_init_options(&channel, &opts, optmask));
   EXPECT_NE(nullptr, channel);
 
-  ares_socket_t socks[3] = {-1, -1, -1};
+  ares_socket_t socks[3] = {ARES_SOCKET_BAD, ARES_SOCKET_BAD, ARES_SOCKET_BAD};
   int bitmask = ares_getsock(channel, socks, 3);
   EXPECT_EQ(0, bitmask);
   bitmask = ares_getsock(channel, nullptr, 0);