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

1 files changed, 4 insertions, 26 deletions

diff --git a/lib/conncache.c b/lib/conncache.c
index 57d6061fd..a23244cf6 100644
--- a/lib/conncache.c
+++ b/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);