[GNUnet-SVN] [gnurl] 65/163: CURLOPT_RESOLVE: always purge old entry first

[gnunet]

This is an automated email from the git hooks/post-receive script.

ng0 pushed a commit to branch master
in repository gnurl.

commit f66d97b677d114733f6b938d54ce7502d6aff214
Author: Alibek.Jorajev <address@hidden>
AuthorDate: Wed May 30 15:23:35 2018 +0100

    CURLOPT_RESOLVE: always purge old entry first
    
    If there's an existing entry using the selected name.
    
    Closes #2622
---
 docs/libcurl/opts/CURLOPT_RESOLVE.3   |  5 ++
 lib/hostip.c                          | 34 +++++++------
 tests/data/Makefile.inc               |  2 +-
 tests/data/test1609                   | 26 ++++++++++
 tests/unit/Makefile.inc               |  5 +-
 tests/unit/README                     |  4 +-
 tests/unit/unit1607.c                 |  8 +++
 tests/unit/{unit1607.c => unit1609.c} | 91 ++++++++++++++++++-----------------
 8 files changed, 111 insertions(+), 64 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/unit1607.c b/tests/unit/unit1609.c
similarity index 73%
copy from tests/unit/unit1607.c
copy to tests/unit/unit1609.c
index 697b8b5c4..2b99bee9b 100644
--- a/tests/unit/unit1607.c
+++ b/tests/unit/unit1609.c
@@ -70,47 +70,45 @@ struct testcase {
 };
 
 
-/* In builds without IPv6 support CURLOPT_RESOLVE should skip over those
-   addresses, so we have to do that as well. */
-static const char skip = 0;
-#ifdef ENABLE_IPV6
-#define IPV6ONLY(x) x
-#else
-#define IPV6ONLY(x) &skip
-#endif
-
-/* CURLOPT_RESOLVE address parsing tests */
+/* 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, 127.0.0.2",
-    "test.com", 80, { NULL, }
-  },
-  { "TEST.com:80:,,127.0.0.1,,,127.0.0.2,,,,::1,,,",
-    "test.com", 80, { "127.0.0.1", "127.0.0.2", IPV6ONLY("::1"), }
-  },
-  { "test.com:80:::1,127.0.0.1",
-    "test.com", 80, { IPV6ONLY("::1"), "127.0.0.1", }
-  },
-  { "test.com:80:[::1],127.0.0.1",
-    "test.com", 80, { IPV6ONLY("::1"), "127.0.0.1", }
-  },
-  { "test.com:80:::1",
-    "test.com", 80, { IPV6ONLY("::1"), }
-  },
-  { "test.com:80:[::1]",
-    "test.com", 80, { IPV6ONLY("::1"), }
-  },
   { "test.com:80:127.0.0.1",
     "test.com", 80, { "127.0.0.1", }
   },
-  { "test.com:80:,127.0.0.1",
-    "test.com", 80, { "127.0.0.1", }
-  },
-  { "test.com:80:127.0.0.1,",
-    "test.com", 80, { "127.0.0.1", }
-  },
-  { "test.com:0:127.0.0.1",
-    "test.com", 0, { "127.0.0.1", }
+  { "test.com:80:127.0.0.2",
+    "test.com", 80, { "127.0.0.2", }
   },
 };
 
@@ -118,22 +116,26 @@ 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);
+    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;
 
-    Curl_hostcache_clean(easy, hostcache);
-    easy->dns.hostcache = hostcache;
-    easy->dns.hostcachetype = HCACHE_GLOBAL;
-
     list = curl_slist_append(NULL, tests[i].optval);
     if(!list)
         goto unit_test_abort;
+
     curl_easy_setopt(easy, CURLOPT_RESOLVE, list);
 
     Curl_loadhostpairs(easy);
@@ -156,9 +158,6 @@ UNITTEST_START
       if(!addr && !tests[i].address[j])
         break;
 
-      if(tests[i].address[j] == &skip)
-        continue;
-
       if(addr && !Curl_getaddressinfo(addr->ai_addr,
                                       ipaddress, &port)) {
         fprintf(stderr, "%s:%d tests[%d] failed. getaddressinfo failed.\n",
@@ -202,7 +201,6 @@ UNITTEST_START
       addr = addr->ai_next;
     }
 
-    Curl_hostcache_clean(easy, easy->dns.hostcache);
     curl_slist_free_all(list);
 
     if(problem) {
@@ -210,4 +208,7 @@ UNITTEST_START
       continue;
     }
   }
+
+  Curl_hostcache_clean(easy, easy->dns.hostcache);
+
 UNITTEST_STOP

-- 
To stop receiving notification emails like this one, please contact
address@hidden