conncache: hold the lock longer when fiddling with the bundles

If the lock is released before the dealings with the bundle is over, it may
have changed by another thread in the mean time.

Fixes #2132

4 files changed, 64 insertions, 32 deletions

diff --git a/lib/conncache.c b/lib/conncache.c
index f8ef2e88b..7750b15d3 100644
--- a/lib/conncache.c
+++ b/lib/conncache.c
@@ -40,11 +40,27 @@
 #include "curl_memory.h"
 #include "memdebug.h"
 
+#ifdef CURLDEBUG
+/* the debug versions of these macros make extra certain that the lock is
+   never doubly locked or unlocked */
+#define CONN_LOCK(x) if((x)->share) {                                   \
+    Curl_share_lock((x), CURL_LOCK_DATA_CONNECT, CURL_LOCK_ACCESS_SINGLE); \
+    DEBUGASSERT(!(x)->state.conncache_lock);                            \
+    (x)->state.conncache_lock = TRUE;                                   \
+  }
+
+#define CONN_UNLOCK(x) if((x)->share) {                                 \
+    DEBUGASSERT((x)->state.conncache_lock);                             \
+    (x)->state.conncache_lock = FALSE;                                  \
+    Curl_share_unlock((x), CURL_LOCK_DATA_CONNECT);                     \
+  }
+
+#else
 #define CONN_LOCK(x) if((x)->share)                                     \
     Curl_share_lock((x), CURL_LOCK_DATA_CONNECT, CURL_LOCK_ACCESS_SINGLE)
 #define CONN_UNLOCK(x) if((x)->share)                   \
     Curl_share_unlock((x), CURL_LOCK_DATA_CONNECT)
-
+#endif
 
 static void conn_llist_dtor(void *user, void *element)
 {
@@ -165,18 +181,24 @@ static void hashkey(struct connectdata *conn, char *buf,
   snprintf(buf, len, "%ld%s", conn->port, hostname);
 }
 
+void Curl_conncache_unlock(struct connectdata *conn)
+{
+  CONN_UNLOCK(conn->data);
+}
+
 /* Look up the bundle with all the connections to the same host this
-   connectdata struct is setup to use. */
+   connectdata struct is setup to use.
+
+   **NOTE**: When it returns, it holds the connection cache lock! */
 struct connectbundle *Curl_conncache_find_bundle(struct connectdata *conn,
                                                  struct conncache *connc)
 {
   struct connectbundle *bundle = NULL;
+  CONN_LOCK(conn->data);
   if(connc) {
     char key[128];
     hashkey(conn, key, sizeof(key));
-    CONN_LOCK(conn->data);
     bundle = Curl_hash_pick(&connc->hash, key, strlen(key));
-    CONN_UNLOCK(conn->data);
   }
 
   return bundle;
@@ -223,36 +245,34 @@ CURLcode Curl_conncache_add_conn(struct conncache *connc,
   struct connectbundle *new_bundle = NULL;
   struct Curl_easy *data = conn->data;
 
+  /* *find_bundle() locks the connection cache */
   bundle = Curl_conncache_find_bundle(conn, data->state.conn_cache);
   if(!bundle) {
     int rc;
     char key[128];
 
     result = bundle_create(data, &new_bundle);
-    if(result)
-      return result;
+    if(result) {
+      goto unlock;
+    }
 
     hashkey(conn, key, sizeof(key));
-    CONN_LOCK(data);
     rc = conncache_add_bundle(data->state.conn_cache, key, new_bundle);
-    CONN_UNLOCK(data);
 
     if(!rc) {
       bundle_destroy(new_bundle);
-      return CURLE_OUT_OF_MEMORY;
+      result = CURLE_OUT_OF_MEMORY;
+      goto unlock;
     }
     bundle = new_bundle;
   }
 
-  CONN_LOCK(data);
   result = bundle_add_conn(bundle, conn);
   if(result) {
     if(new_bundle)
       conncache_remove_bundle(data->state.conn_cache, new_bundle);
-    CONN_UNLOCK(data);
-    return result;
+    goto unlock;
   }
-  CONN_UNLOCK(data);
 
   conn->connection_id = connc->next_connection_id++;
   connc->num_connections++;
@@ -261,6 +281,9 @@ CURLcode Curl_conncache_add_conn(struct conncache *connc,
                "The cache now contains %" CURL_FORMAT_CURL_OFF_TU " members\n",
                conn->connection_id, (curl_off_t) connc->num_connections));
 
+  unlock:
+  CONN_UNLOCK(data);
+
   return CURLE_OK;
 }
 
diff --git a/lib/conncache.h b/lib/conncache.h
index 0d97a6cef..6a1ffecdd 100644
--- a/lib/conncache.h
+++ b/lib/conncache.h
@@ -50,6 +50,7 @@ void Curl_conncache_destroy(struct conncache *connc);
 /* return the correct bundle, to a host or a proxy */
 struct connectbundle *Curl_conncache_find_bundle(struct connectdata *conn,
                                                  struct conncache *connc);
+void Curl_conncache_unlock(struct connectdata *conn);
 
 CURLcode Curl_conncache_add_conn(struct conncache *connc,
                                  struct connectdata *conn);
diff --git a/lib/url.c b/lib/url.c
index 731e67e17..765717358 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -1106,8 +1106,8 @@ ConnectionExists(struct Curl_easy *data,
      Curl_pipeline_site_blacklisted(data, needle))
     canpipe &= ~ CURLPIPE_HTTP1;
 
-  /* Look up the bundle with all the connections to this
-     particular host */
+  /* Look up the bundle with all the connections to this particular host.
+     Locks the connection cache, beware of early returns! */
   bundle = Curl_conncache_find_bundle(needle, data->state.conn_cache);
   if(bundle) {
     /* Max pipe length is zero (unlimited) for multiplexed connections */
@@ -1130,6 +1130,7 @@ ConnectionExists(struct Curl_easy *data,
         if((bundle->multiuse == BUNDLE_UNKNOWN) && data->set.pipewait) {
           infof(data, "Server doesn't support multi-use yet, wait\n");
           *waitpipe = TRUE;
+          Curl_conncache_unlock(needle);
           return FALSE; /* no re-use */
         }
 
@@ -1477,6 +1478,7 @@ ConnectionExists(struct Curl_easy *data,
       }
     }
   }
+  Curl_conncache_unlock(needle);
 
   if(chosen) {
     *usethis = chosen;
@@ -4455,7 +4457,6 @@ static CURLcode create_conn(struct Curl_easy *data,
     /* We have decided that we want a new connection. However, we may not
        be able to do that if we have reached the limit of how many
        connections we are allowed to open. */
-    struct connectbundle *bundle = NULL;
 
     if(conn->handler->flags & PROTOPT_ALPN_NPN) {
       /* The protocol wants it, so set the bits if enabled in the easy handle
@@ -4470,26 +4471,30 @@ static CURLcode create_conn(struct Curl_easy *data,
       /* There is a connection that *might* become usable for pipelining
          "soon", and we wait for that */
       connections_available = FALSE;
-    else
-      bundle = Curl_conncache_find_bundle(conn, data->state.conn_cache);
+    else {
+      /* this gets a lock on the conncache */
+      struct connectbundle *bundle =
+        Curl_conncache_find_bundle(conn, data->state.conn_cache);
 
-    if(max_host_connections > 0 && bundle &&
-       (bundle->num_connections >= max_host_connections)) {
-      struct connectdata *conn_candidate;
+      if(max_host_connections > 0 && bundle &&
+         (bundle->num_connections >= max_host_connections)) {
+        struct connectdata *conn_candidate;
 
-      /* The bundle is full. Let's see if we can kill a connection. */
-      conn_candidate = find_oldest_idle_connection_in_bundle(data, bundle);
+        /* The bundle is full. Let's see if we can kill a connection. */
+        conn_candidate = find_oldest_idle_connection_in_bundle(data, bundle);
 
-      if(conn_candidate) {
-        /* Set the connection's owner correctly, then kill it */
-        conn_candidate->data = data;
-        (void)Curl_disconnect(conn_candidate, /* dead_connection */ FALSE);
-      }
-      else {
-        infof(data, "No more connections allowed to host: %d\n",
-              max_host_connections);
-        connections_available = FALSE;
+        if(conn_candidate) {
+          /* Set the connection's owner correctly, then kill it */
+          conn_candidate->data = data;
+          (void)Curl_disconnect(conn_candidate, /* dead_connection */ FALSE);
+        }
+        else {
+          infof(data, "No more connections allowed to host: %d\n",
+                max_host_connections);
+          connections_available = FALSE;
+        }
       }
+      Curl_conncache_unlock(conn);
     }
 
     if(connections_available &&
diff --git a/lib/urldata.h b/lib/urldata.h
index ed6bbb4f0..894cf3fa9 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -1325,6 +1325,9 @@ struct UrlState {
   struct Curl_easy *stream_depends_on;
   bool stream_depends_e; /* set or don't set the Exclusive bit */
   int stream_weight;
+#ifdef CURLDEBUG
+  bool conncache_lock;
+#endif
 };