- Constantine Sapuntzakis brought a patch:

  The problem mentioned on Dec 10 2009
  (http://curl.haxx.se/bug/view.cgi?id=2905220) was only partially fixed.
  Partially because an easy handle can be associated with many connections in
  the cache (e.g. if there is a redirect during the lifetime of the easy
  handle).  The previous patch only cleaned up the first one. The new fix now
  removes the easy handle from all connections, not just the first one.

3 files changed, 83 insertions, 68 deletions

diff --git a/CHANGES b/CHANGES
index 320923b9a..4075b8843 100644
--- a/CHANGES
+++ b/CHANGES
@@ -6,6 +6,16 @@
 
                                   Changelog
 
+Daniel Stenberg (15 Mar 2010)
+- Constantine Sapuntzakis brought a patch:
+
+  The problem mentioned on Dec 10 2009
+  (http://curl.haxx.se/bug/view.cgi?id=2905220) was only partially fixed.
+  Partially because an easy handle can be associated with many connections in
+  the cache (e.g. if there is a redirect during the lifetime of the easy
+  handle).  The previous patch only cleaned up the first one. The new fix now
+  removes the easy handle from all connections, not just the first one.
+
 Daniel Stenberg (6 Mar 2010)
 - Ben Greear brought a patch that fixed the rate limiting logic for TFTP when
   the easy interface was used.
diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index 2ecaf6aa0..e03fa2f39 100644
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -31,6 +31,7 @@ This release includes the following bugfixes:
  o threaded resolver double free when closing curl handle
  o configure fixes for building with the clang compiler
  o easy interix rate limiting logic
+ o curl_multi_remove_handle() caused use after free
 
 This release includes the following known bugs:
 
@@ -41,6 +42,7 @@ advice from friends like these:
 
  Steven M. Schweda, Yang Tse, Jack Zhang, Tom Donovan, Martin Hager,
  Daniel Fandrich, Patrick Monnerat, Pat Ray, Wesley Miaw, Ben Greear,
- Ryan Chan, Markus Duft, Andrei Benea, Jacob Moshenko, Daniel Johnson
+ Ryan Chan, Markus Duft, Andrei Benea, Jacob Moshenko, Daniel Johnson,
+ Constantine Sapuntzakis
 
         Thanks! (and sorry if I forgot to mention someone)
diff --git a/lib/multi.c b/lib/multi.c
index e4e1ce8c9..b24021280 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -181,12 +181,12 @@ struct Curl_multi {
                                     previous callback */
 };
 
-static struct connectdata *conn_using(struct Curl_multi *multi,
+static void multi_connc_remove_handle(struct Curl_multi *multi,
                                       struct SessionHandle *data);
 static void singlesocket(struct Curl_multi *multi,
                          struct Curl_one_easy *easy);
-static void add_closure(struct Curl_multi *multi,
-                        struct SessionHandle *data);
+static CURLMcode add_closure(struct Curl_multi *multi,
+                             struct SessionHandle *data);
 static int update_timer(struct Curl_multi *multi);
 
 static CURLcode addHandleToSendOrPendPipeline(struct SessionHandle *handle,
@@ -576,7 +576,6 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle,
 {
   struct Curl_multi *multi=(struct Curl_multi *)multi_handle;
   struct Curl_one_easy *easy;
-  struct connectdata *conn;
 
   /* First, make some basic checks that the CURLM handle is a good handle */
   if(!GOOD_MULTI_HANDLE(multi))
@@ -649,42 +648,9 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle,
         Curl_getoff_all_pipelines(easy->easy_handle, easy->easy_conn);
     }
 
-    /* figure out if the easy handle is used by a connection in the cache */
-    conn = conn_using(multi, easy->easy_handle);
-
-    /* If this easy_handle was the last one in charge for one or more
-       connections in the shared connection cache, we might need to keep this
-       handle around until either A) the connection is closed and killed
-       properly, or B) another easy_handle uses the connection.
-
-       The reason why we need to have a easy_handle associated with a live
-       connection is simply that some connections will need a handle to get
-       closed down properly. Currently, the only connections that need to keep
-       a easy_handle handle around are using FTP(S). Such connections have
-       the PROT_CLOSEACTION bit set.
-
-       Thus, we need to check for all connections in the shared cache that
-       points to this handle and are using PROT_CLOSEACTION. If there's any,
-       we need to add this handle to the list of "easy handles kept around for
-       nice connection closures".
-    */
-    if(conn) {
-      if(conn->protocol & PROT_CLOSEACTION) {
-        /* There's at least one CLOSEACTION connection using this handle so we
-           must keep this handle around. We also keep the connection cache
-           pointer pointing to the shared one since that will be used on close
-           as well. */
-        easy->easy_handle->state.shared_conn = multi;
-
-        /* this handle is still being used by a shared connection cache and
-           thus we leave it around for now */
-        add_closure(multi, easy->easy_handle);
-      }
-      else
-        /* disconect the easy handle from the connection since the connection
-           will now remain but this easy handle is going */
-        conn->data = NULL;
-    }
+    /* figure out if the easy handle is used by one or more connections in the
+       cache */
+    multi_connc_remove_handle(multi, easy->easy_handle);
 
     if(easy->easy_handle->state.connc->type == CONNCACHE_MULTI) {
       /* if this was using the shared connection cache we clear the pointer
@@ -2373,48 +2339,71 @@ CURLMcode curl_multi_assign(CURLM *multi_handle,
   return CURLM_OK;
 }
 
-static struct connectdata *conn_using(struct Curl_multi *multi,
+static void multi_connc_remove_handle(struct Curl_multi *multi,
                                       struct SessionHandle *data)
 {
   /* a connection in the connection cache pointing to the given 'data' ? */
   int i;
 
   for(i=0; i< multi->connc->num; i++) {
-    if(multi->connc->connects[i] &&
-       (multi->connc->connects[i]->data == data))
-      return multi->connc->connects[i];
-  }
+    struct connectdata * conn = multi->connc->connects[i];
+
+    if(conn && conn->data == data) {
+      /* If this easy_handle was the last one in charge for one or more
+         connections in the shared connection cache, we might need to keep
+         this handle around until either A) the connection is closed and
+         killed properly, or B) another easy_handle uses the connection.
+
+         The reason why we need to have a easy_handle associated with a live
+         connection is simply that some connections will need a handle to get
+         closed down properly. Currently, the only connections that need to
+         keep a easy_handle handle around are using FTP(S). Such connections
+         have the PROT_CLOSEACTION bit set.
+
+         Thus, we need to check for all connections in the shared cache that
+         points to this handle and are using PROT_CLOSEACTION. If there's any,
+         we need to add this handle to the list of "easy handles kept around
+         for nice connection closures".
+      */
 
-  return NULL;
+      if(conn->protocol & PROT_CLOSEACTION) {
+        /* this handle is still being used by a shared connection and
+           thus we leave it around for now */
+        if(add_closure(multi, data) == CURLM_OK)
+          data->state.shared_conn = multi;
+        else {
+          /* out of memory - so much for graceful shutdown */
+          Curl_disconnect(conn);
+          multi->connc->connects[i] = NULL;
+        }
+      }
+      else
+        /* disconect the easy handle from the connection since the connection
+           will now remain but this easy handle is going */
+        conn->data = NULL;
+    }
+  }
 }
 
 /* Add the given data pointer to the list of 'closure handles' that are kept
    around only to be able to close some connections nicely - just make sure
    that this handle isn't already added, like for the cases when an easy
    handle is removed, added and removed again... */
-static void add_closure(struct Curl_multi *multi,
-                        struct SessionHandle *data)
+static CURLMcode add_closure(struct Curl_multi *multi,
+                             struct SessionHandle *data)
 {
-  int i;
-  struct closure *cl = calloc(1, sizeof(struct closure));
-  struct closure *p=NULL;
-  struct closure *n;
-  if(cl) {
-    cl->easy_handle = data;
-    cl->next = multi->closure;
-    multi->closure = cl;
-  }
-
-  p = multi->closure;
-  cl = p->next; /* start immediately on the second since the first is the one
-                   we just added and it is _very_ likely to actually exist
-                   used in the cache since that's the whole purpose of adding
-                   it to this list! */
+  struct closure *cl = multi->closure;
+  struct closure *p = NULL;
+  bool add = TRUE;
 
-  /* When adding, scan through all the other currently kept handles and see if
-     there are any connections still referring to them and kill them if not. */
+  /* Before adding, scan through all the other currently kept handles and see
+     if there are any connections still referring to them and kill them if
+     not. */
   while(cl) {
+    struct closure *n;
     bool inuse = FALSE;
+    int i;
+
     for(i=0; i< multi->connc->num; i++) {
       if(multi->connc->connects[i] &&
          (multi->connc->connects[i]->data == cl->easy_handle)) {
@@ -2436,13 +2425,27 @@ static void add_closure(struct Curl_multi *multi,
       else
         multi->closure = n;
       free(cl);
-    }
-    else
+    } else {
+      if(cl->easy_handle == data)
+        add = FALSE;
+
       p = cl;
+    }
 
     cl = n;
   }
 
+  if (add) {
+    cl = calloc(1, sizeof(struct closure));
+    if(!cl)
+      return CURLM_OUT_OF_MEMORY;
+
+    cl->easy_handle = data;
+    cl->next = multi->closure;
+    multi->closure = cl;
+  }
+
+  return CURLM_OK;
 }
 
 #ifdef DEBUGBUILD