conncache: more protection of inuse and the bundle accesses

7 files changed, 61 insertions, 15 deletions

diff --git a/lib/conncache.c b/lib/conncache.c
index 89aa26451..412c57961 100644
--- a/lib/conncache.c
+++ b/lib/conncache.c
@@ -199,6 +199,18 @@ size_t Curl_conncache_size(struct Curl_easy *data)
   return num;
 }
 
+/* Returns number of connections currently held in the connections's bundle
+   Locks/unlocks the cache itself!
+*/
+size_t Curl_conncache_bundle_size(struct connectdata *conn)
+{
+  size_t num;
+  CONN_LOCK(conn->data);
+  num = conn->bundle->num_connections;
+  CONN_UNLOCK(conn->data);
+  return num;
+}
+
 /* Look up the bundle with all the connections to the same host this
    connectdata struct is setup to use.
 
@@ -315,6 +327,7 @@ void Curl_conncache_remove_conn(struct connectdata *conn, bool lock)
     bundle_remove_conn(bundle, conn);
     if(bundle->num_connections == 0)
       conncache_remove_bundle(connc, bundle);
+    conn->bundle = NULL; /* removed from it */
     if(connc) {
       connc->num_conn--;
       DEBUGF(infof(conn->data, "The cache now contains %"
@@ -418,7 +431,6 @@ Curl_conncache_find_first_connection(struct conncache *connc)
 bool Curl_conncache_return_conn(struct connectdata *conn)
 {
   struct Curl_easy *data = conn->data;
-  struct conncache *connc = data->state.conn_cache;
 
   /* data->multi->maxconnects can be negative, deal with it. */
   size_t maxconnects =
@@ -493,7 +505,7 @@ Curl_conncache_extract_bundle(struct Curl_easy *data,
     data->state.conn_cache->num_conn--;
     DEBUGF(infof(data, "The cache now contains %"
                  CURL_FORMAT_CURL_OFF_TU " members\n",
-                 (curl_off_t) connc->num_conn));
+                 (curl_off_t) data->state.conn_cache->num_conn));
   }
 
   return conn_candidate;
diff --git a/lib/conncache.h b/lib/conncache.h
index 208af27dd..d8ad80f96 100644
--- a/lib/conncache.h
+++ b/lib/conncache.h
@@ -59,6 +59,7 @@ struct connectbundle *Curl_conncache_find_bundle(struct connectdata *conn,
 void Curl_conncache_unlock(struct connectdata *conn);
 /* returns number of connections currently held in the connection cache */
 size_t Curl_conncache_size(struct Curl_easy *data);
+size_t Curl_conncache_bundle_size(struct connectdata *conn);
 
 bool Curl_conncache_return_conn(struct connectdata *conn);
 CURLcode Curl_conncache_add_conn(struct conncache *connc,
diff --git a/lib/http.c b/lib/http.c
index def51abc3..7c00718dd 100644
--- a/lib/http.c
+++ b/lib/http.c
@@ -1069,6 +1069,8 @@ CURLcode Curl_add_buffer_send(Curl_send_buffer *in,
 
   DEBUGASSERT(socketindex <= SECONDARYSOCKET);
 
+  Curl_check_conn(conn);
+
   sockfd = conn->sock[socketindex];
 
   /* The looping below is required since we use non-blocking sockets, but due
@@ -1766,6 +1768,8 @@ CURLcode Curl_http(struct connectdata *conn, bool *done)
   curl_off_t postsize = 0; /* curl_off_t to handle large file sizes */
   int seekerr = CURL_SEEKFUNC_CANTSEEK;
 
+  Curl_check_conn(conn);
+
   /* Always consider the DO phase done after this function call, even if there
      may be parts of the request that is not yet sent, since we can deal with
      the rest of the request in the PERFORM phase. */
@@ -2306,6 +2310,8 @@ CURLcode Curl_http(struct connectdata *conn, bool *done)
   if(result)
     return result;
 
+  Curl_check_conn(conn);
+
   result =
     Curl_add_bufferf(req_buffer,
                      "%s" /* ftp typecode (;type=x) */
@@ -2952,6 +2958,8 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data,
   CURLcode result;
   struct SingleRequest *k = &data->req;
 
+  Curl_check_conn(conn);
+
   /* header line within buffer loop */
   do {
     size_t rest_length;
diff --git a/lib/multi.c b/lib/multi.c
index 615d7c7aa..333421fe3 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -142,8 +142,10 @@ static void mstate(struct Curl_easy *data, CURLMstate state
      data->mstate < CURLM_STATE_COMPLETED) {
     long connection_id = -5000;
 
-    if(data->easy_conn)
+    if(data->easy_conn) {
+      Curl_check_conn(data->easy_conn);
       connection_id = data->easy_conn->connection_id;
+    }
 
     infof(data,
           "STATE: %s => %s handle %p; line %d (connection #%ld)\n",
@@ -1329,16 +1331,18 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
     }
 
     if(data->easy_conn && data->mstate > CURLM_STATE_CONNECT &&
-       data->mstate < CURLM_STATE_COMPLETED)
+       data->mstate < CURLM_STATE_COMPLETED) {
       /* Make sure we set the connection's current owner */
+      Curl_check_conn(data->easy_conn);
       data->easy_conn->data = data;
+    }
 
     if(data->easy_conn &&
        (data->mstate >= CURLM_STATE_CONNECT) &&
        (data->mstate < CURLM_STATE_COMPLETED)) {
       /* we need to wait for the connect state as only then is the start time
          stored, but we must not check already completed handles */
-
+      Curl_check_conn(data->easy_conn);
       timeout_ms = Curl_timeleft(data, &now,
                                  (data->mstate <= CURLM_STATE_WAITDO)?
                                  TRUE:FALSE);
diff --git a/lib/url.c b/lib/url.c
index 33d9a1cbf..a9328290b 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -718,7 +718,9 @@ static void conn_free(struct connectdata *conn)
 #ifdef USE_SSL
   Curl_safefree(conn->ssl_extra);
 #endif
-
+#ifdef CURLDEBUG
+  conn->magic = 0;
+#endif
   free(conn); /* free all the connection oriented data */
 }
 
@@ -1449,12 +1451,15 @@ ConnectionExists(struct Curl_easy *data,
       }
     }
   }
-  Curl_conncache_unlock(needle);
 
   if(chosen) {
+    /* mark it as used before releasing the lock */
+    chosen->inuse = TRUE;
+    Curl_conncache_unlock(needle);
     *usethis = chosen;
     return TRUE; /* yes, we found one to use! */
   }
+  Curl_conncache_unlock(needle);
 
   if(foundPendingCandidate && data->set.pipewait) {
     infof(data,
@@ -1777,6 +1782,9 @@ static struct connectdata *allocate_conn(struct Curl_easy *data)
   if(!conn)
     return NULL;
 
+#ifdef CURLDEBUG
+  conn->magic = CONN_MAGIC;
+#endif
 #ifdef USE_SSL
   /* The SSL backend-specific data (ssl_backend_data) objects are allocated as
      a separate array to ensure suitable alignment.
@@ -4382,20 +4390,21 @@ static CURLcode create_conn(struct Curl_easy *data,
   else
     reuse = ConnectionExists(data, conn, &conn_temp, &force_reuse, &waitpipe);
 
-  /* If we found a reusable connection, we may still want to
-     open a new connection if we are pipelining. */
+  /* If we found a reusable connection that is now marked as in use, we may
+     still want to open a new connection if we are pipelining. */
   if(reuse && !force_reuse && IsPipeliningPossible(data, conn_temp)) {
     size_t pipelen = conn_temp->send_pipe.size + conn_temp->recv_pipe.size;
     if(pipelen > 0) {
       infof(data, "Found connection %ld, with requests in the pipe (%zu)\n",
             conn_temp->connection_id, pipelen);
 
-      if(conn_temp->bundle->num_connections < max_host_connections &&
+      if(Curl_conncache_bundle_size(conn_temp) < max_host_connections &&
          Curl_conncache_size(data) < max_total_connections) {
         /* We want a new connection anyway */
         reuse = FALSE;
 
         infof(data, "We can reuse, but we want a new connection anyway\n");
+        Curl_conncache_return_conn(conn_temp);
       }
     }
   }
@@ -4407,12 +4416,13 @@ static CURLcode create_conn(struct Curl_easy *data,
      * just allocated before we can move along and use the previously
      * existing one.
      */
-    conn_temp->inuse = TRUE; /* mark this as being in use so that no other
-                                handle in a multi stack may nick it */
     reuse_conn(conn, conn_temp);
 #ifdef USE_SSL
     free(conn->ssl_extra);
 #endif
+#ifdef CURLDEBUG
+    conn->magic = 0;
+#endif
     free(conn);          /* we don't need this anymore */
     conn = conn_temp;
     *in_connect = conn;
diff --git a/lib/url.h b/lib/url.h
index 5dd04fdff..f161da109 100644
--- a/lib/url.h
+++ b/lib/url.h
@@ -94,4 +94,11 @@ void Curl_verboseconnect(struct connectdata *conn);
   (conn->http_proxy.proxytype == CURLPROXY_HTTPS &&\
   !conn->bits.proxy_ssl_connected[SECONDARYSOCKET])
 
+#ifdef CURLDEBUG
+#define CONN_MAGIC 0x1234abcd
+#define Curl_check_conn(x) DEBUGASSERT((x)->magic == CONN_MAGIC)
+#else
+#define Curl_check_conn(x)
+#endif
+
 #endif /* HEADER_CURL_URL_H */
diff --git a/lib/urldata.h b/lib/urldata.h
index 894cf3fa9..31fa314ce 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -775,9 +775,10 @@ struct connectdata {
   void *closesocket_client;
 
   bool inuse; /* This is a marker for the connection cache logic. If this is
-                 TRUE this handle is being used by an easy handle and cannot
-                 be used by any other easy handle without careful
-                 consideration (== only for pipelining). */
+                 TRUE this handle is being used by one or more easy handles
+                 and can only used by any other easy handle without careful
+                 consideration (== only for pipelining/multiplexing) and it
+                 cannot be used by another multi handle! */
 
   /**** Fields set when inited and not modified again */
   long connection_id; /* Contains a unique number to make it easier to
@@ -1007,6 +1008,9 @@ struct connectdata {
   char *unix_domain_socket;
   bool abstract_unix_socket;
 #endif
+#ifdef CURLDEBUG
+  unsigned int magic;
+#endif
 };
 
 /* The end of connectdata. */