- Dmitry Popov filed bug report #1911069

  (http://curl.haxx.se/bug/view.cgi?id=1911069) that identified a race
  condition in the name resolver code when the DNS cache is shared between
  multiple easy handles, each running in simultaneous threads that could cause
  crashes.

3 files changed, 24 insertions, 28 deletions

diff --git a/CHANGES b/CHANGES
index cd041504d..5f62bcafc 100644
--- a/CHANGES
+++ b/CHANGES
@@ -7,6 +7,12 @@
                                   Changelog
 
 Daniel Stenberg (11 Mar 2008)
+- Dmitry Popov filed bug report #1911069
+  (http://curl.haxx.se/bug/view.cgi?id=1911069) that identified a race
+  condition in the name resolver code when the DNS cache is shared between
+  multiple easy handles, each running in simultaneous threads that could cause
+  crashes.
+
 - Added a macro for curl_easy_setopt() that accepts three arguments and simply
   does nothing with them, just to make sure libcurl users always use three
   arguments to this function. Due to its use of ... for the third argument, it
diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index 0c20a0701..8dccd0d48 100644
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -42,6 +42,8 @@ This release includes the following bugfixes:
  o better MIT kerberos configure check
  o curl_easy_reset() + SFTP re-used connection download crash
  o SFTP non-existing file + SFTP existing file error
+ o sharing DNS cache between easy handles running in multiple threads could
+   lead to crash
 
 This release includes the following known bugs:
 
@@ -61,6 +63,6 @@ advice from friends like these:
  Michal Marek, Dmitry Kurochkin, Niklas Angebrand, Günter Knauf, Yang Tse,
  Dan Fandrich, Mike Hommey, Pooyan McSporran, Jerome Muffat-Meridol,
  Kaspar Brand, Gautam Kachroo, Zmey Petroff, Georg Lippitsch, Sam Listopad,
- Anatoli Tubman, Mike Protts, Michael Calmer, Brian Ulm
+ Anatoli Tubman, Mike Protts, Michael Calmer, Brian Ulm, Dmitry Popov
 
         Thanks! (and sorry if I forgot to mention someone)
diff --git a/lib/hostip.c b/lib/hostip.c
index 0778fc222..82ea6a6ab 100644
--- a/lib/hostip.c
+++ b/lib/hostip.c
@@ -268,6 +268,9 @@ void Curl_hostcache_prune(struct SessionHandle *data)
     Curl_share_unlock(data, CURL_LOCK_DATA_DNS);
 }
 
+/*
+ * Check if the entry should be pruned. Assumes a locked cache.
+ */
 static int
 remove_entry_if_stale(struct SessionHandle *data, struct Curl_dns_entry *dns)
 {
@@ -284,19 +287,10 @@ remove_entry_if_stale(struct SessionHandle *data, struct Curl_dns_entry *dns)
   if( !hostcache_timestamp_remove(&user,dns) )
     return 0;
 
-  /* ok, we do need to clear the cache. although we need to remove just a
-     single entry we clean the entire hash, as no explicit delete function
-     is provided */
-  if(data->share)
-    Curl_share_lock(data, CURL_LOCK_DATA_DNS, CURL_LOCK_ACCESS_SINGLE);
-
   Curl_hash_clean_with_criterium(data->dns.hostcache,
                                  (void *) &user,
                                  hostcache_timestamp_remove);
 
-  if(data->share)
-    Curl_share_unlock(data, CURL_LOCK_DATA_DNS);
-
   return 1;
 }
 
@@ -397,7 +391,7 @@ int Curl_resolv(struct connectdata *conn,
   size_t entry_len;
   struct SessionHandle *data = conn->data;
   CURLcode result;
-  int rc;
+  int rc = CURLRESOLV_ERROR; /* default to failure */
   *entry = NULL;
 
 #ifdef HAVE_SIGSETJMP
@@ -407,7 +401,7 @@ int Curl_resolv(struct connectdata *conn,
     if(sigsetjmp(curl_jmpenv, 1)) {
       /* this is coming from a siglongjmp() */
       failf(data, "name lookup timed out");
-      return CURLRESOLV_ERROR;
+      return rc;
     }
   }
 #endif
@@ -416,7 +410,7 @@ int Curl_resolv(struct connectdata *conn,
   entry_id = create_hostcache_id(hostname, port);
   /* If we can't create the entry id, fail */
   if(!entry_id)
-    return CURLRESOLV_ERROR;
+    return rc;
 
   entry_len = strlen(entry_id);
 
@@ -426,19 +420,21 @@ int Curl_resolv(struct connectdata *conn,
   /* See if its already in our dns cache */
   dns = Curl_hash_pick(data->dns.hostcache, entry_id, entry_len+1);
 
+  /* See whether the returned entry is stale. Done before we release lock */
+  if( remove_entry_if_stale(data, dns) )
+    dns = NULL; /* the memory deallocation is being handled by the hash */
+
+  if(dns) {
+    dns->inuse++; /* we use it! */
+    rc = CURLRESOLV_RESOLVED;
+  }
+
   if(data->share)
     Curl_share_unlock(data, CURL_LOCK_DATA_DNS);
 
   /* free the allocated entry_id again */
   free(entry_id);
 
-  /* See whether the returned entry is stale. Deliberately done after the
-     locked block */
-  if( remove_entry_if_stale(data,dns) )
-    dns = NULL; /* the memory deallocation is being handled by the hash */
-
-  rc = CURLRESOLV_ERROR; /* default to failure */
-
   if(!dns) {
     /* The entry was not in the cache. Resolve it to IP address */
 
@@ -486,14 +482,6 @@ int Curl_resolv(struct connectdata *conn,
         rc = CURLRESOLV_RESOLVED;
     }
   }
-  else {
-    if(data->share)
-      Curl_share_lock(data, CURL_LOCK_DATA_DNS, CURL_LOCK_ACCESS_SINGLE);
-    dns->inuse++; /* we use it! */
-    if(data->share)
-      Curl_share_unlock(data, CURL_LOCK_DATA_DNS);
-    rc = CURLRESOLV_RESOLVED;
-  }
 
   *entry = dns;