conncache: fix multi-thread use of shared connection cache

It could accidentally let the connection get used by more than one
thread, leading to double-free and more.

Reported-by: Christopher Reid
Fixes #4544
Closes #4557

lib/conncache.c

@@ -40,27 +40,6 @@
 #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
-
 #define HASHKEY_SIZE 128
 
 static void conn_llist_dtor(void *user, void *element)
@@ -122,6 +101,7 @@ static int bundle_remove_conn(struct connectbundle *cb_ptr,
     }
     curr = curr->next;
   }
+  DEBUGASSERT(0);
   return 0;
 }
 
@@ -428,17 +408,15 @@ conncache_find_first_connection(struct conncache *connc)
  *
  * Return TRUE if stored, FALSE if closed.
  */
-bool Curl_conncache_return_conn(struct connectdata *conn)
+bool Curl_conncache_return_conn(struct Curl_easy *data,
+                                struct connectdata *conn)
 {
-  struct Curl_easy *data = conn->data;
-
   /* data->multi->maxconnects can be negative, deal with it. */
   size_t maxconnects =
     (data->multi->maxconnects < 0) ? data->multi->num_easy * 4:
     data->multi->maxconnects;
   struct connectdata *conn_candidate = NULL;
 
-  conn->data = NULL; /* no owner anymore */
   conn->lastused = Curl_now(); /* it was used up until now */
   if(maxconnects > 0 &&
      Curl_conncache_size(data) > maxconnects) {
@@ -541,7 +519,7 @@ Curl_conncache_extract_oldest(struct Curl_easy *data)
     while(curr) {
       conn = curr->ptr;
 
-      if(!CONN_INUSE(conn) && !conn->data) {
+      if(!CONN_INUSE(conn) && !conn->data && !conn->bits.close) {
         /* Set higher score for the age passed since the connection was used */
         score = Curl_timediff(now, conn->lastused);
 

lib/conncache.h

@@ -42,6 +42,27 @@ struct conncache {
 #define BUNDLE_UNKNOWN     0  /* initial value */
 #define BUNDLE_MULTIPLEX   2
 
+#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
+
 struct connectbundle {
   int multiuse;                 /* supports multi-use */
   size_t num_connections;       /* Number of connections in the bundle */
@@ -61,7 +82,8 @@ void Curl_conncache_unlock(struct Curl_easy *data);
 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);
+bool Curl_conncache_return_conn(struct Curl_easy *data,
+                                struct connectdata *conn);
 CURLcode Curl_conncache_add_conn(struct conncache *connc,
                                  struct connectdata *conn) WARN_UNUSED_RESULT;
 void Curl_conncache_remove_conn(struct Curl_easy *data,

lib/http.c

@@ -1617,7 +1617,7 @@ CURLcode Curl_http_done(struct connectdata *conn,
     Curl_add_buffer_free(&http->send_buffer);
   }
 
-  Curl_http2_done(conn, premature);
+  Curl_http2_done(data, premature);
   Curl_quic_done(data, premature);
 
   Curl_mime_cleanpart(&http->form);

lib/http2.c

@@ -1169,11 +1169,10 @@ static void populate_settings(struct connectdata *conn,
   httpc->local_settings_num = 3;
 }
 
-void Curl_http2_done(struct connectdata *conn, bool premature)
+void Curl_http2_done(struct Curl_easy *data, bool premature)
 {
-  struct Curl_easy *data = conn->data;
   struct HTTP *http = data->req.protop;
-  struct http_conn *httpc = &conn->proto.httpc;
+  struct http_conn *httpc = &data->conn->proto.httpc;
 
   /* there might be allocated resources done before this got the 'h2' pointer
      setup */

lib/http2.h

@@ -50,7 +50,7 @@ CURLcode Curl_http2_switched(struct connectdata *conn,
 /* called from http_setup_conn */
 void Curl_http2_setup_conn(struct connectdata *conn);
 void Curl_http2_setup_req(struct Curl_easy *data);
-void Curl_http2_done(struct connectdata *conn, bool premature);
+void Curl_http2_done(struct Curl_easy *data, bool premature);
 CURLcode Curl_http2_done_sending(struct connectdata *conn);
 CURLcode Curl_http2_add_child(struct Curl_easy *parent,
                               struct Curl_easy *child,

lib/multi.c

@@ -547,6 +547,8 @@ static CURLcode multi_done(struct Curl_easy *data,
     /* Stop if multi_done() has already been called */
     return CURLE_OK;
 
+  conn->data = data; /* ensure the connection uses this transfer now */
+
   /* Stop the resolver and free its own resources (but not dns_entry yet). */
   Curl_resolver_kill(conn);
 
@@ -583,15 +585,17 @@ static CURLcode multi_done(struct Curl_easy *data,
 
   process_pending_handles(data->multi); /* connection / multiplex */
 
+  CONN_LOCK(data);
   detach_connnection(data);
   if(CONN_INUSE(conn)) {
     /* Stop if still used. */
+    CONN_UNLOCK(data);
     DEBUGF(infof(data, "Connection still in use %zu, "
                  "no more multi_done now!\n",
                  conn->easyq.size));
     return CURLE_OK;
   }
-
+  conn->data = NULL; /* the connection now has no owner */
   data->state.done = TRUE; /* called just now! */
 
   if(conn->dns_entry) {
@@ -634,7 +638,10 @@ static CURLcode multi_done(struct Curl_easy *data,
 #endif
      ) || conn->bits.close
        || (premature && !(conn->handler->flags & PROTOPT_STREAM))) {
-    CURLcode res2 = Curl_disconnect(data, conn, premature);
+    CURLcode res2;
+    conn->bits.close = TRUE; /* forcibly prevents reuse */
+    CONN_UNLOCK(data);
+    res2 = Curl_disconnect(data, conn, premature);
 
     /* If we had an error already, make sure we return that one. But
        if we got a new error, return that. */
@@ -651,9 +658,9 @@ static CURLcode multi_done(struct Curl_easy *data,
               conn->bits.httpproxy ? conn->http_proxy.host.dispname :
               conn->bits.conn_to_host ? conn->conn_to_host.dispname :
               conn->host.dispname);
-
     /* the connection is no longer in use by this transfer */
-    if(Curl_conncache_return_conn(conn)) {
+    CONN_UNLOCK(data);
+    if(Curl_conncache_return_conn(data, conn)) {
       /* remember the most recently used connection */
       data->state.lastconnect = conn;
       infof(data, "%s\n", buffer);
@@ -760,10 +767,8 @@ CURLMcode curl_multi_remove_handle(struct Curl_multi *multi,
                                 vanish with this handle */
 
   /* Remove the association between the connection and the handle */
-  if(data->conn) {
+  if(data->conn)
-    data->conn->data = NULL;
     detach_connnection(data);
-  }
 
 #ifdef USE_LIBPSL
   /* Remove the PSL association. */
@@ -1349,6 +1354,7 @@ static CURLcode multi_do(struct Curl_easy *data, bool *done)
 
   DEBUGASSERT(conn);
   DEBUGASSERT(conn->handler);
+  DEBUGASSERT(conn->data == data);
 
   if(conn->handler->do_it) {
     /* generic protocol-specific function pointer set in curl_connect() */

lib/url.c

@@ -1082,16 +1082,15 @@ ConnectionExists(struct Curl_easy *data,
       check = curr->ptr;
       curr = curr->next;
 
-      if(check->bits.connect_only)
+      if(check->bits.connect_only || check->bits.close)
-        /* connect-only connections will not be reused */
+        /* connect-only or to-be-closed connections will not be reused */
         continue;
 
       multiplexed = CONN_INUSE(check) &&
         (bundle->multiuse == BUNDLE_MULTIPLEX);
 
       if(canmultiplex) {
-        if(check->bits.protoconnstart && check->bits.close)
+        ;
-          continue;
       }
       else {
         if(multiplexed) {
@@ -1111,12 +1110,9 @@ ConnectionExists(struct Curl_easy *data,
           }
         }
 
-        if((check->sock[FIRSTSOCKET] == CURL_SOCKET_BAD) ||
+        if(check->sock[FIRSTSOCKET] == CURL_SOCKET_BAD) {
-           check->bits.close) {
+          foundPendingCandidate = TRUE;
-          if(!check->bits.close)
+          /* Don't pick a connection that hasn't connected yet */
-            foundPendingCandidate = TRUE;
-          /* Don't pick a connection that hasn't connected yet or that is going
-             to get closed. */
           infof(data, "Connection #%ld isn't open enough, can't reuse\n",
                 check->connection_id);
           continue;
@@ -1194,8 +1190,7 @@ ConnectionExists(struct Curl_easy *data,
            already in use so we skip it */
         continue;
 
-      if(CONN_INUSE(check) && check->data &&
+      if(check->data && (check->data->multi != needle->data->multi))
-         (check->data->multi != needle->data->multi))
         /* this could be subject for multiplex use, but only if they belong to
          * the same multi handle */
         continue;
@@ -1643,6 +1638,7 @@ static struct connectdata *allocate_conn(struct Curl_easy *data)
      it may live on without (this specific) Curl_easy */
   conn->fclosesocket = data->set.fclosesocket;
   conn->closesocket_client = data->set.closesocket_client;
+  conn->lastused = Curl_now(); /* used now */
 
   return conn;
   error:
@@ -3612,7 +3608,6 @@ static CURLcode create_conn(struct Curl_easy *data,
         reuse = FALSE;
 
         infof(data, "We can reuse, but we want a new connection anyway\n");
-        Curl_conncache_return_conn(conn_temp);
       }
     }
   }

tests/data/test1554

@@ -38,6 +38,8 @@ run 1: foobar and so on fun!
 <- Mutex unlock
 -> Mutex lock
 <- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
 run 1: foobar and so on fun!
 -> Mutex lock
 <- Mutex unlock
@@ -47,6 +49,8 @@ run 1: foobar and so on fun!
 <- Mutex unlock
 -> Mutex lock
 <- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
 run 1: foobar and so on fun!
 -> Mutex lock
 <- Mutex unlock
@@ -54,6 +58,8 @@ run 1: foobar and so on fun!
 <- Mutex unlock
 -> Mutex lock
 <- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
 </datacheck>
 </reply>