CURLOPT_RESOLVE: always purge old entry first

If there's an existing entry using the selected name.

Closes #2622

8 files changed, 279 insertions, 19 deletions

diff --git a/docs/libcurl/opts/CURLOPT_RESOLVE.3 b/docs/libcurl/opts/CURLOPT_RESOLVE.3
index b4d79cf3b..d5f8ded64 100644
--- a/docs/libcurl/opts/CURLOPT_RESOLVE.3
+++ b/docs/libcurl/opts/CURLOPT_RESOLVE.3
@@ -50,6 +50,11 @@ HOST+PORT will instead use your provided ADDRESS. Addresses set with
 \fICURLOPT_RESOLVE(3)\fP will not time-out from the DNS cache like ordinary
 entries.
 
+If the DNS cache already have an entry for the given host+port pair, then
+this entry will be removed and a new entry will be created. This is because
+old entry may have have different addresses or be ordinary entries with
+time-outs.
+
 The provided ADDRESS set by this option will be used even if
 \fICURLOPT_IPRESOLVE(3)\fP is set to make libcurl use another IP version.
 
diff --git a/lib/hostip.c b/lib/hostip.c
index c2f9defd9..d809578e1 100644
--- a/lib/hostip.c
+++ b/lib/hostip.c
@@ -1010,24 +1010,28 @@ CURLcode Curl_loadhostpairs(struct Curl_easy *data)
       /* See if its already in our dns cache */
       dns = Curl_hash_pick(data->dns.hostcache, entry_id, entry_len + 1);
 
+      if(dns) {
+        infof(data, "RESOLVE %s:%d is - old addresses discarded!\n",
+                hostname, port);
+        /* delete old entry entry, there are two reasons for this
+         1. old entry may have different addresses.
+         2. even if entry with correct addresses is already in the cache,
+            but if it is close to expire, then by the time next http
+            request is made, it can get expired and pruned because old
+            entry is not necessarily marked as added by CURLOPT_RESOLVE. */
+
+        Curl_hash_delete(data->dns.hostcache, entry_id, entry_len + 1);
+      }
       /* free the allocated entry_id again */
       free(entry_id);
 
-      if(!dns) {
-        /* if not in the cache already, put this host in the cache */
-        dns = Curl_cache_addr(data, head, hostname, port);
-        if(dns) {
-          dns->timestamp = 0; /* mark as added by CURLOPT_RESOLVE */
-          /* release the returned reference; the cache itself will keep the
-           * entry alive: */
-          dns->inuse--;
-        }
-      }
-      else {
-        /* this is a duplicate, free it again */
-        infof(data, "RESOLVE %s:%d is already cached, %s not stored!\n",
-              hostname, port, addresses);
-        Curl_freeaddrinfo(head);
+      /* put this new host in the cache */
+      dns = Curl_cache_addr(data, head, hostname, port);
+      if(dns) {
+        dns->timestamp = 0; /* mark as added by CURLOPT_RESOLVE */
+        /* release the returned reference; the cache itself will keep the
+         * entry alive: */
+            dns->inuse--;
       }
 
       if(data->share)
diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc
index 0b487ec68..c7dcaaa27 100644
--- a/tests/data/Makefile.inc
+++ b/tests/data/Makefile.inc
@@ -177,7 +177,7 @@ test1533 test1534 test1535 test1536 test1537 test1538 \
 test1540 \
 test1550 test1551 test1552 test1553 test1554 test1555 test1556 \
 test1600 test1601 test1602 test1603 test1604 test1605 test1606 test1607 \
-test1608 \
+test1608 test1609 \
 \
 test1700 test1701 test1702 \
 \
diff --git a/tests/data/test1609 b/tests/data/test1609
new file mode 100644
index 000000000..c1b7c7a11
--- /dev/null
+++ b/tests/data/test1609
@@ -0,0 +1,26 @@
+<testcase>
+<info>
+<keywords>
+unittest
+CURLOPT_RESOLVE
+</keywords>
+</info>
+
+#
+# Client-side
+<client>
+<server>
+none
+</server>
+<features>
+unittest
+</features>
+ <name>
+CURLOPT_RESOLVE parsing
+ </name>
+<tool>
+unit1609
+</tool>
+</client>
+
+</testcase>
diff --git a/tests/unit/Makefile.inc b/tests/unit/Makefile.inc
index f77da7588..a2618813f 100644
--- a/tests/unit/Makefile.inc
+++ b/tests/unit/Makefile.inc
@@ -10,7 +10,7 @@ UNITPROGS = unit1300 unit1301 unit1302 unit1303 unit1304 unit1305 unit1307	\
  unit1330 unit1394 unit1395 unit1396 unit1397 unit1398	\
  unit1399	\
  unit1600 unit1601 unit1602 unit1603 unit1604 unit1605 unit1606 unit1607 \
- unit1608
+ unit1608 unit1609
 
 unit1300_SOURCES = unit1300.c $(UNITFILES)
 unit1300_CPPFLAGS = $(AM_CPPFLAGS)
@@ -92,3 +92,6 @@ unit1607_CPPFLAGS = $(AM_CPPFLAGS)
 
 unit1608_SOURCES = unit1608.c $(UNITFILES)
 unit1608_CPPFLAGS = $(AM_CPPFLAGS)
+
+unit1609_SOURCES = unit1609.c $(UNITFILES)
+unit1609_CPPFLAGS = $(AM_CPPFLAGS)
diff --git a/tests/unit/README b/tests/unit/README
index 301cd17d5..b8a513b3b 100644
--- a/tests/unit/README
+++ b/tests/unit/README
@@ -17,8 +17,8 @@ Run Unit Tests
 
 Unit tests are run as part of the regular test suite. If you have built
 everything to run unit tests, to can do 'make test' at the root level. Or you
-can 'cd tests' and then invoke individual unit tests with ./runtests.pl NNNN
-where NNNN is the specific test number.
+can 'cd tests' and 'make' and then invoke individual unit tests with
+./runtests.pl NNNN where NNNN is the specific test number.
 
 Debug Unit Tests
 ================
diff --git a/tests/unit/unit1607.c b/tests/unit/unit1607.c
index 697b8b5c4..64b6371ee 100644
--- a/tests/unit/unit1607.c
+++ b/tests/unit/unit1607.c
@@ -199,6 +199,14 @@ UNITTEST_START
         break;
       }
 
+      if(dns->timestamp != 0) {
+        fprintf(stderr, "%s:%d tests[%d] failed. the timestamp is not zero. "
+                "for tests[%d].address[%d\n",
+                __FILE__, __LINE__, i, i, j);
+        problem = true;
+        break;
+      }
+
       addr = addr->ai_next;
     }
 
diff --git a/tests/unit/unit1609.c b/tests/unit/unit1609.c
new file mode 100644
index 000000000..2b99bee9b
--- /dev/null
+++ b/tests/unit/unit1609.c
@@ -0,0 +1,214 @@
+/***************************************************************************
+ *                                  _   _ ____  _
+ *  Project                     ___| | | |  _ \| |
+ *                             / __| | | | |_) | |
+ *                            | (__| |_| |  _ <| |___
+ *                             \___|\___/|_| \_\_____|
+ *
+ * Copyright (C) 1998 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.
+ *
+ * This software is licensed as described in the file COPYING, which
+ * you should have received as part of this distribution. The terms
+ * are also available at https://curl.haxx.se/docs/copyright.html.
+ *
+ * You may opt to use, copy, modify, merge, publish, distribute and/or sell
+ * copies of the Software, and permit persons to whom the Software is
+ * furnished to do so, under the terms of the COPYING file.
+ *
+ * This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY
+ * KIND, either express or implied.
+ *
+ ***************************************************************************/
+#include "curlcheck.h"
+
+#include "urldata.h"
+#include "connect.h"
+#include "share.h"
+
+#include "memdebug.h" /* LAST include file */
+
+static struct Curl_easy *easy;
+static struct curl_hash *hostcache;
+
+static void unit_stop(void)
+{
+  curl_easy_cleanup(easy);
+  curl_global_cleanup();
+}
+
+static CURLcode unit_setup(void)
+{
+  int res = CURLE_OK;
+
+  global_init(CURL_GLOBAL_ALL);
+
+  easy = curl_easy_init();
+  if(!easy) {
+    curl_global_cleanup();
+    return CURLE_OUT_OF_MEMORY;
+  }
+
+  hostcache = Curl_global_host_cache_init();
+  if(!hostcache) {
+    unit_stop();
+    return CURLE_OUT_OF_MEMORY;
+  }
+
+  return res;
+}
+
+struct testcase {
+  /* host:port:address[,address]... */
+  const char *optval;
+
+  /* lowercase host and port to retrieve the addresses from hostcache */
+  const char *host;
+  int port;
+
+  /* 0 to 9 addresses expected from hostcache */
+  const char *address[10];
+};
+
+
+/* CURLOPT_RESOLVE address parsing test - to test the following defect fix:
+
+ 1) if there is already existing host:port pair in the DNS cache and
+ we call CURLOPT_RESOLVE, it should also replace addresses.
+ for example, if there is "test.com:80" with address "1.1.1.1"
+ and we called CURLOPT_RESOLVE with address "2.2.2.2", then DNS entry needs to
+ reflect that.
+
+ 2) when cached address is already there and close to expire, then by the
+ time request is made, it can get expired.  This happens because, when
+ we set address using CURLOPT_RESOLVE,
+ it usually marks as permanent (by setting timestamp to zero). However,
+ if address already exists
+in the cache, then it does not mark it, but just leaves it as it is.
+ So we fixing this by timestamp to zero if address already exists too.
+
+Test:
+
+ - insert new entry
+ - verify that timestamp is not zero
+ - call set options with CURLOPT_RESOLVE
+ - then, call Curl_loadhostpairs
+
+ expected result: cached address has zero timestamp.
+
+ - call set options with CURLOPT_RESOLVE with same host:port pair,
+   different address.
+ - then, call Curl_loadhostpairs
+
+ expected result: cached address has zero timestamp and new address
+*/
+
+static const struct testcase tests[] = {
+  /* spaces aren't allowed, for now */
+  { "test.com:80:127.0.0.1",
+    "test.com", 80, { "127.0.0.1", }
+  },
+  { "test.com:80:127.0.0.2",
+    "test.com", 80, { "127.0.0.2", }
+  },
+};
+
+UNITTEST_START
+  int i;
+  int testnum = sizeof(tests) / sizeof(struct testcase);
+
+/* important: we setup cache outside of the loop
+  and also clean cache after the loop. In contrast,for example,
+  test 1607 sets up and cleans cache on each iteration. */
+  Curl_hostcache_clean(easy, hostcache);
+  easy->dns.hostcache = hostcache;
+  easy->dns.hostcachetype = HCACHE_GLOBAL;
+
+  for(i = 0; i < testnum; ++i, curl_easy_reset(easy)) {
+    int j;
+    int addressnum = sizeof (tests[i].address) / sizeof (*tests[i].address);
+    struct Curl_addrinfo *addr;
+    struct Curl_dns_entry *dns;
+    struct curl_slist *list;
+    void *entry_id;
+    bool problem = false;
+
+    list = curl_slist_append(NULL, tests[i].optval);
+    if(!list)
+        goto unit_test_abort;
+
+    curl_easy_setopt(easy, CURLOPT_RESOLVE, list);
+
+    Curl_loadhostpairs(easy);
+
+    entry_id = (void *)aprintf("%s:%d", tests[i].host, tests[i].port);
+    if(!entry_id) {
+      curl_slist_free_all(list);
+      goto unit_test_abort;
+    }
+    dns = Curl_hash_pick(easy->dns.hostcache, entry_id, strlen(entry_id) + 1);
+    free(entry_id);
+    entry_id = NULL;
+
+    addr = dns ? dns->addr : NULL;
+
+    for(j = 0; j < addressnum; ++j) {
+      long port = 0;
+      char ipaddress[MAX_IPADR_LEN] = {0};
+
+      if(!addr && !tests[i].address[j])
+        break;
+
+      if(addr && !Curl_getaddressinfo(addr->ai_addr,
+                                      ipaddress, &port)) {
+        fprintf(stderr, "%s:%d tests[%d] failed. getaddressinfo failed.\n",
+                __FILE__, __LINE__, i);
+        problem = true;
+        break;
+      }
+
+      if(addr && !tests[i].address[j]) {
+        fprintf(stderr, "%s:%d tests[%d] failed. the retrieved addr "
+                "is %s but tests[%d].address[%d] is NULL.\n",
+                __FILE__, __LINE__, i, ipaddress, i, j);
+        problem = true;
+        break;
+      }
+
+      if(!addr && tests[i].address[j]) {
+        fprintf(stderr, "%s:%d tests[%d] failed. the retrieved addr "
+                "is NULL but tests[%d].address[%d] is %s.\n",
+                __FILE__, __LINE__, i, i, j, tests[i].address[j]);
+        problem = true;
+        break;
+      }
+
+      if(!curl_strequal(ipaddress, tests[i].address[j])) {
+        fprintf(stderr, "%s:%d tests[%d] failed. the retrieved addr "
+                "%s is not equal to tests[%d].address[%d] %s.\n",
+                __FILE__, __LINE__, i, ipaddress, i, j, tests[i].address[j]);
+        problem = true;
+        break;
+      }
+
+      if(port != tests[i].port) {
+        fprintf(stderr, "%s:%d tests[%d] failed. the retrieved port "
+                "for tests[%d].address[%d] is %ld but tests[%d].port is %d.\n",
+                __FILE__, __LINE__, i, i, j, port, i, tests[i].port);
+        problem = true;
+        break;
+      }
+
+      addr = addr->ai_next;
+    }
+
+    curl_slist_free_all(list);
+
+    if(problem) {
+      unitfail++;
+      continue;
+    }
+  }
+
+  Curl_hostcache_clean(easy, easy->dns.hostcache);
+
+UNITTEST_STOP