- Lots of good work by Krister Johansen, mostly related to pipelining:

  Fix SIGSEGV on free'd easy_conn when pipe unexpectedly breaks
  Fix data corruption issue with re-connected transfers
  Fix use after free if we're completed but easy_conn not NULL

9 files changed, 213 insertions, 87 deletions

diff --git a/CHANGES b/CHANGES
index 5ecc26330..650a36f7b 100644
--- a/CHANGES
+++ b/CHANGES
@@ -6,6 +6,13 @@
 
                                   Changelog
 
+Daniel Stenberg (21 Aug 2009)
+- Lots of good work by Krister Johansen, mostly related to pipelining:
+
+  Fix SIGSEGV on free'd easy_conn when pipe unexpectedly breaks
+  Fix data corruption issue with re-connected transfers
+  Fix use after free if we're completed but easy_conn not NULL
+
 Kamil Dudka (13 Aug 2009)
 - Changed NSS code to not ignore the value of ssl.verifyhost and produce more
   verbose error messages. Originally reported at:
diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index f38ff7091..5af32e16a 100644
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -14,6 +14,10 @@ This release includes the following changes:
 This release includes the following bugfixes:
 
  o The windows makefiles work again
+ o libcurl-NSS acknowledges verifyhost
+ o SIGSEGV when pipelined pipe unexpectedly breaks
+ o data corruption issue with re-connected transfers
+ o use after free if we're completed but easy_conn not NULL (pipelined)
 
 This release includes the following known bugs:
 
@@ -22,6 +26,6 @@ This release includes the following known bugs:
 This release would not have looked like this without help, code, reports and
 advice from friends like these:
 
- Karl Moerder
+ Karl Moerder, Kamil Dudka, Krister Johansen,
 
         Thanks! (and sorry if I forgot to mention someone)
diff --git a/TODO-RELEASE b/TODO-RELEASE
index 4897358cf..5c4add331 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -10,9 +10,6 @@ To be addressed in 7.19.7 (planned release: October 2009)
 254 - Problem re-using easy handle after call to curl_multi_remove_handle
       http://curl.haxx.se/mail/lib-2009-07/0249.html
     
-255 - debugging a crash in Curl_pgrsTime/checkPendPipeline?
-      http://curl.haxx.se/mail/lib-2009-08/0066.html
-
 256 - "More questions about ares behavior"
       http://curl.haxx.se/mail/lib-2009-08/0012.html
 
diff --git a/docs/KNOWN_BUGS b/docs/KNOWN_BUGS
index 474991d90..35ca80a84 100644
--- a/docs/KNOWN_BUGS
+++ b/docs/KNOWN_BUGS
@@ -12,9 +12,6 @@ may have been fixed since this was written!
 70. Problem re-using easy handle after call to curl_multi_remove_handle
   http://curl.haxx.se/mail/lib-2009-07/0249.html
     
-69. debugging a crash in Curl_pgrsTime/checkPendPipeline?
-  http://curl.haxx.se/mail/lib-2009-08/0066.html
-
 68. "More questions about ares behavior".
   http://curl.haxx.se/mail/lib-2009-08/0012.html
 
diff --git a/lib/multi.c b/lib/multi.c
index 0f75c2d72..1099b525d 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -192,7 +192,9 @@ static int update_timer(struct Curl_multi *multi);
 static CURLcode addHandleToSendOrPendPipeline(struct SessionHandle *handle,
                                               struct connectdata *conn);
 static int checkPendPipeline(struct connectdata *conn);
-static void moveHandleFromSendToRecvPipeline(struct SessionHandle *habdle,
+static void moveHandleFromSendToRecvPipeline(struct SessionHandle *handle,
+                                             struct connectdata *conn);
+static void moveHandleFromRecvToDonePipeline(struct SessionHandle *handle,
                                              struct connectdata *conn);
 static bool isHandleAtHead(struct SessionHandle *handle,
                            struct curl_llist *pipeline);
@@ -233,14 +235,16 @@ static void multistate(struct Curl_one_easy *easy, CURLMstate state)
   easy->state = state;
 
 #ifdef DEBUGBUILD
-  if(easy->state > CURLM_STATE_CONNECT &&
-     easy->state < CURLM_STATE_COMPLETED)
-    connectindex = easy->easy_conn->connectindex;
+  if(easy->easy_conn) {
+    if(easy->state > CURLM_STATE_CONNECT &&
+       easy->state < CURLM_STATE_COMPLETED)
+      connectindex = easy->easy_conn->connectindex;
 
-  infof(easy->easy_handle,
-        "STATE: %s => %s handle %p; (connection #%ld) \n",
-        statename[oldstate], statename[easy->state],
-        (char *)easy, connectindex);
+    infof(easy->easy_handle,
+          "STATE: %s => %s handle %p; (connection #%ld) \n",
+          statename[oldstate], statename[easy->state],
+          (char *)easy, connectindex);
+  }
 #endif
   if(state == CURLM_STATE_COMPLETED)
     /* changing to COMPLETED means there's one less easy handle 'alive' */
@@ -925,7 +929,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
       break;
     }
 
-    if(easy->state > CURLM_STATE_CONNECT &&
+    if(easy->easy_conn && easy->state > CURLM_STATE_CONNECT &&
        easy->state < CURLM_STATE_COMPLETED)
       /* Make sure we set the connection's current owner */
       easy->easy_conn->data = easy->easy_handle;
@@ -1149,7 +1153,6 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
                                &dophase_done);
 
         if(CURLE_OK == easy->result) {
-
           if(!dophase_done) {
             /* DO was not completed in one function call, we must continue
                DOING... */
@@ -1170,6 +1173,49 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
             result = CURLM_CALL_MULTI_PERFORM;
           }
         }
+        else if ((CURLE_SEND_ERROR == easy->result) &&
+                 easy->easy_conn->bits.reuse) {
+          /*
+           * In this situation, a connection that we were trying to use
+           * may have unexpectedly died.  If possible, send the connection
+           * back to the CONNECT phase so we can try again.
+           */
+          char *newurl;
+          followtype follow=FOLLOW_NONE;
+          CURLcode drc;
+          bool retry = Curl_retry_request(easy->easy_conn, &newurl);
+
+          Curl_posttransfer(easy->easy_handle);
+          drc = Curl_done(&easy->easy_conn, easy->result, FALSE);
+
+          /* When set to retry the connection, we must to go back to
+           * the CONNECT state */
+          if(retry) {
+            if ((drc == CURLE_OK) || (drc == CURLE_SEND_ERROR)) {
+              follow = FOLLOW_RETRY;
+              drc = Curl_follow(easy->easy_handle, newurl, follow);
+              if(drc == CURLE_OK) {
+                multistate(easy, CURLM_STATE_CONNECT);
+                result = CURLM_CALL_MULTI_PERFORM;
+                easy->result = CURLE_OK;
+              }
+              else {
+                /* Follow failed */
+                easy->result = drc;
+                free(newurl);
+              }
+            }
+            else {
+              /* done didn't return OK or SEND_ERROR */
+              easy->result = drc;
+              free(newurl);
+            }
+          }
+          else {
+            /* Have error handler disconnect conn if we can't retry */
+            disconnect_conn = TRUE;
+          }
+        }
         else {
           /* failure detected */
           Curl_posttransfer(easy->easy_handle);
@@ -1331,8 +1377,8 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         Curl_posttransfer(easy->easy_handle);
 
         /* we're no longer receving */
-        Curl_removeHandleFromPipeline(easy->easy_handle,
-                                      easy->easy_conn->recv_pipe);
+        moveHandleFromRecvToDonePipeline(easy->easy_handle,
+                                         easy->easy_conn);
 
         /* expire the new receiving pipeline head */
         if(easy->easy_conn->recv_pipe->head)
@@ -1386,21 +1432,35 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
       break;
 
     case CURLM_STATE_DONE:
-      /* Remove ourselves from the receive pipeline */
-      Curl_removeHandleFromPipeline(easy->easy_handle,
-                                    easy->easy_conn->recv_pipe);
-      /* Check if we can move pending requests to send pipe */
-      checkPendPipeline(easy->easy_conn);
 
-      if(easy->easy_conn->bits.stream_was_rewound) {
-        /* This request read past its response boundary so we quickly let the
-           other requests consume those bytes since there is no guarantee that
-           the socket will become active again */
-        result = CURLM_CALL_MULTI_PERFORM;
-      }
+      if(easy->easy_conn) {
+        /* Remove ourselves from the receive and done pipelines. Handle
+           should be on one of these lists, depending upon how we got here. */
+        Curl_removeHandleFromPipeline(easy->easy_handle,
+                                      easy->easy_conn->recv_pipe);
+        Curl_removeHandleFromPipeline(easy->easy_handle,
+                                      easy->easy_conn->done_pipe);
+        /* Check if we can move pending requests to send pipe */
+        checkPendPipeline(easy->easy_conn);
+
+        if(easy->easy_conn->bits.stream_was_rewound) {
+          /* This request read past its response boundary so we quickly let
+             the other requests consume those bytes since there is no
+             guarantee that the socket will become active again */
+          result = CURLM_CALL_MULTI_PERFORM;
+        }
 
-      /* post-transfer command */
-      easy->result = Curl_done(&easy->easy_conn, CURLE_OK, FALSE);
+        /* post-transfer command */
+        easy->result = Curl_done(&easy->easy_conn, CURLE_OK, FALSE);
+        /*
+         * If there are other handles on the pipeline, Curl_done won't set
+         * easy_conn to NULL.  In such a case, curl_multi_remove_handle() can
+         * access free'd data, if the connection is free'd and the handle
+         * removed before we perform the processing in CURLM_STATE_COMPLETED
+         */
+        if (easy->easy_conn)
+          easy->easy_conn = NULL;
+      }
 
       /* after we have DONE what we're supposed to do, go COMPLETED, and
          it doesn't matter what the Curl_done() returned! */
@@ -1443,6 +1503,8 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
                                         easy->easy_conn->send_pipe);
           Curl_removeHandleFromPipeline(easy->easy_handle,
                                         easy->easy_conn->recv_pipe);
+          Curl_removeHandleFromPipeline(easy->easy_handle,
+                                        easy->easy_conn->done_pipe);
           /* Check if we can move pending requests to send pipe */
           checkPendPipeline(easy->easy_conn);
         }
@@ -2173,6 +2235,21 @@ static void moveHandleFromSendToRecvPipeline(struct SessionHandle *handle,
   }
 }
 
+static void moveHandleFromRecvToDonePipeline(struct SessionHandle *handle,
+                                            struct connectdata *conn)
+{
+  struct curl_llist_element *curr;
+
+  curr = conn->recv_pipe->head;
+  while(curr) {
+    if(curr->ptr == handle) {
+      Curl_llist_move(conn->recv_pipe, curr,
+                      conn->done_pipe, conn->done_pipe->tail);
+      break;
+    }
+    curr = curr->next;
+  }
+}
 static bool isHandleAtHead(struct SessionHandle *handle,
                            struct curl_llist *pipeline)
 {
diff --git a/lib/transfer.c b/lib/transfer.c
index d59b3f500..e5ba279d0 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -176,7 +176,7 @@ CURLcode Curl_fillreadbuffer(struct connectdata *conn, int bytes, int *nreadp)
   }
 
   if(!data->req.forbidchunk && data->req.upload_chunky) {
-    /* if chunked Transfer-Encoding 
+    /* if chunked Transfer-Encoding
      *    build chunk:
      *
      *        <HEX SIZE> CRLF
@@ -217,9 +217,9 @@ CURLcode Curl_fillreadbuffer(struct connectdata *conn, int bytes, int *nreadp)
     /* copy the prefix to the buffer, leaving out the NUL */
     memcpy(data->req.upload_fromhere, hexbuffer, hexlen);
 
-    /* always append ASCII CRLF to the data */  
-    memcpy(data->req.upload_fromhere + nread, 
-           endofline_network, 
+    /* always append ASCII CRLF to the data */
+    memcpy(data->req.upload_fromhere + nread,
+           endofline_network,
            strlen(endofline_network));
 
 #ifdef CURL_DOES_CONVERSIONS
@@ -2495,6 +2495,61 @@ connect_host(struct SessionHandle *data,
   return res;
 }
 
+CURLcode
+Curl_reconnect_request(struct connectdata **connp)
+{
+  CURLcode result = CURLE_OK;
+  struct connectdata *conn = *connp;
+  struct SessionHandle *data = conn->data;
+
+  /* This was a re-use of a connection and we got a write error in the
+   * DO-phase. Then we DISCONNECT this connection and have another attempt to
+   * CONNECT and then DO again! The retry cannot possibly find another
+   * connection to re-use, since we only keep one possible connection for
+   * each.  */
+
+  infof(data, "Re-used connection seems dead, get a new one\n");
+
+  conn->bits.close = TRUE; /* enforce close of this connection */
+  result = Curl_done(&conn, result, FALSE); /* we are so done with this */
+
+  /* conn may no longer be a good pointer */
+
+  /*
+   * According to bug report #1330310. We need to check for CURLE_SEND_ERROR
+   * here as well. I figure this could happen when the request failed on a FTP
+   * connection and thus Curl_done() itself tried to use the connection
+   * (again). Slight Lack of feedback in the report, but I don't think this
+   * extra check can do much harm.
+   */
+  if((CURLE_OK == result) || (CURLE_SEND_ERROR == result)) {
+    bool async;
+    bool protocol_done = TRUE;
+
+    /* Now, redo the connect and get a new connection */
+    result = Curl_connect(data, connp, &async, &protocol_done);
+    if(CURLE_OK == result) {
+      /* We have connected or sent away a name resolve query fine */
+
+      conn = *connp; /* setup conn to again point to something nice */
+      if(async) {
+        /* Now, if async is TRUE here, we need to wait for the name
+           to resolve */
+        result = Curl_wait_for_resolv(conn, NULL);
+        if(result)
+          return result;
+
+        /* Resolved, continue with the connection */
+        result = Curl_async_resolved(conn, &protocol_done);
+        if(result)
+          return result;
+      }
+    }
+  }
+
+  return result;
+}
+
 /* Returns TRUE and sets '*url' if a request retry is wanted.
 
    NOTE: that the *url is malloc()ed. */
diff --git a/lib/transfer.h b/lib/transfer.h
index aad82ebaf..4b39faa18 100644
--- a/lib/transfer.h
+++ b/lib/transfer.h
@@ -46,6 +46,7 @@ int Curl_single_getsock(const struct connectdata *conn,
                         int numsocks);
 CURLcode Curl_readrewind(struct connectdata *conn);
 CURLcode Curl_fillreadbuffer(struct connectdata *conn, int bytes, int *nreadp);
+CURLcode Curl_reconnect_request(struct connectdata **connp);
 bool Curl_retry_request(struct connectdata *conn, char **url);
 
 /* This sets up a forthcoming transfer */
diff --git a/lib/url.c b/lib/url.c
index ae2917548..011f6c4d7 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -146,7 +146,7 @@ void idn_free (void *ptr); /* prototype from idn-free.h, not provided by
 /* Local static prototypes */
 static long ConnectionKillOne(struct SessionHandle *data);
 static void conn_free(struct connectdata *conn);
-static void signalPipeClose(struct curl_llist *pipeline);
+static void signalPipeClose(struct curl_llist *pipeline, bool pipe_broke);
 
 #ifdef CURL_DISABLE_VERBOSE_STRINGS
 #define verboseconnect(x)  do { } while (0)
@@ -420,6 +420,16 @@ CURLcode Curl_close(struct SessionHandle *data)
           }
         }
       }
+      pipeline = connptr->done_pipe;
+      if(pipeline) {
+        for (curr = pipeline->head; curr; curr=curr->next) {
+          if(data == (struct SessionHandle *) curr->ptr) {
+            fprintf(stderr,
+                    "MAJOR problem we %p are still in done pipe for %p done %d\n",
+                    data, connptr, (int)connptr->bits.done);
+          }
+        }
+      }
       pipeline = connptr->pend_pipe;
       if(pipeline) {
         for (curr = pipeline->head; curr; curr=curr->next) {
@@ -2316,6 +2326,7 @@ static void conn_free(struct connectdata *conn)
   Curl_llist_destroy(conn->send_pipe, NULL);
   Curl_llist_destroy(conn->recv_pipe, NULL);
   Curl_llist_destroy(conn->pend_pipe, NULL);
+  Curl_llist_destroy(conn->done_pipe, NULL);
 
   /* possible left-overs from the async name resolvers */
 #if defined(USE_ARES)
@@ -2415,9 +2426,10 @@ CURLcode Curl_disconnect(struct connectdata *conn)
 
   /* Indicate to all handles on the pipe that we're dead */
   if(Curl_isPipeliningEnabled(data)) {
-    signalPipeClose(conn->send_pipe);
-    signalPipeClose(conn->recv_pipe);
-    signalPipeClose(conn->pend_pipe);
+    signalPipeClose(conn->send_pipe, TRUE);
+    signalPipeClose(conn->recv_pipe, TRUE);
+    signalPipeClose(conn->pend_pipe, TRUE);
+    signalPipeClose(conn->done_pipe, FALSE);
   }
 
   conn_free(conn);
@@ -2535,9 +2547,10 @@ void Curl_getoff_all_pipelines(struct SessionHandle *data,
   if(Curl_removeHandleFromPipeline(data, conn->send_pipe) && send_head)
     conn->writechannel_inuse = FALSE;
   Curl_removeHandleFromPipeline(data, conn->pend_pipe);
+  Curl_removeHandleFromPipeline(data, conn->done_pipe);
 }
 
-static void signalPipeClose(struct curl_llist *pipeline)
+static void signalPipeClose(struct curl_llist *pipeline, bool pipe_broke)
 {
   struct curl_llist_element *curr;
 
@@ -2556,7 +2569,8 @@ static void signalPipeClose(struct curl_llist *pipeline)
     }
 #endif
 
-    data->state.pipe_broke = TRUE;
+    if (pipe_broke)
+       data->state.pipe_broke = TRUE;
     Curl_multi_handlePipeBreak(data);
     Curl_llist_remove(pipeline, curr, NULL);
     curr = next;
@@ -4217,6 +4231,7 @@ static void reuse_conn(struct connectdata *old_conn,
   Curl_llist_destroy(old_conn->send_pipe, NULL);
   Curl_llist_destroy(old_conn->recv_pipe, NULL);
   Curl_llist_destroy(old_conn->pend_pipe, NULL);
+  Curl_llist_destroy(old_conn->done_pipe, NULL);
   Curl_safefree(old_conn->master_buffer);
 }
 
@@ -4319,7 +4334,9 @@ static CURLcode create_conn(struct SessionHandle *data,
   conn->send_pipe = Curl_llist_alloc((curl_llist_dtor) llist_dtor);
   conn->recv_pipe = Curl_llist_alloc((curl_llist_dtor) llist_dtor);
   conn->pend_pipe = Curl_llist_alloc((curl_llist_dtor) llist_dtor);
-  if(!conn->send_pipe || !conn->recv_pipe || !conn->pend_pipe)
+  conn->done_pipe = Curl_llist_alloc((curl_llist_dtor) llist_dtor);
+  if(!conn->send_pipe || !conn->recv_pipe || !conn->pend_pipe ||
+    !conn->done_pipe)
     return CURLE_OUT_OF_MEMORY;
 
   /* This initing continues below, see the comment "Continue connectdata
@@ -5001,53 +5018,22 @@ CURLcode Curl_do(struct connectdata **connp, bool *done)
 
     /* This was formerly done in transfer.c, but we better do it here */
     if((CURLE_SEND_ERROR == result) && conn->bits.reuse) {
-      /* This was a re-use of a connection and we got a write error in the
-       * DO-phase. Then we DISCONNECT this connection and have another attempt
-       * to CONNECT and then DO again! The retry cannot possibly find another
-       * connection to re-use, since we only keep one possible connection for
-       * each.  */
-
-      infof(data, "Re-used connection seems dead, get a new one\n");
-
-      conn->bits.close = TRUE; /* enforce close of this connection */
-      result = Curl_done(&conn, result, FALSE); /* we are so done with this */
-
-      /* conn may no longer be a good pointer */
+        /*
+         * If the connection is using an easy handle, call reconnect
+         * to re-establish the connection.  Otherwise, let the multi logic
+         * figure out how to re-establish the connection.
+         */
+        if(!data->multi) {
+          result = Curl_reconnect_request(connp);
 
-      /*
-       * According to bug report #1330310. We need to check for
-       * CURLE_SEND_ERROR here as well. I figure this could happen when the
-       * request failed on a FTP connection and thus Curl_done() itself tried
-       * to use the connection (again). Slight Lack of feedback in the report,
-       * but I don't think this extra check can do much harm.
-       */
-      if((CURLE_OK == result) || (CURLE_SEND_ERROR == result)) {
-        bool async;
-        bool protocol_done = TRUE;
-
-        /* Now, redo the connect and get a new connection */
-        result = Curl_connect(data, connp, &async, &protocol_done);
-        if(CURLE_OK == result) {
-          /* We have connected or sent away a name resolve query fine */
-
-          conn = *connp; /* setup conn to again point to something nice */
-          if(async) {
-            /* Now, if async is TRUE here, we need to wait for the name
-               to resolve */
-            result = Curl_wait_for_resolv(conn, NULL);
-            if(result)
-              return result;
-
-            /* Resolved, continue with the connection */
-            result = Curl_async_resolved(conn, &protocol_done);
-            if(result)
-              return result;
+          if(result == CURLE_OK) {
+            /* ... finally back to actually retry the DO phase */
+            result = conn->handler->do_it(conn, done);
           }
-
-          /* ... finally back to actually retry the DO phase */
-          result = conn->handler->do_it(conn, done);
         }
-      }
+        else {
+          return result;
+        }
     }
 
     if((result == CURLE_OK) && *done)
diff --git a/lib/urldata.h b/lib/urldata.h
index a48fc91b7..1ee6637d2 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -1028,6 +1028,8 @@ struct connectdata {
                                    their responses on this pipeline */
   struct curl_llist *pend_pipe; /* List of pending handles on
                                    this pipeline */
+  struct curl_llist *done_pipe; /* Handles that are finished, but
+				   still reference this connectdata */
 #define MAX_PIPELINE_LENGTH 5
 
   char* master_buffer; /* The master buffer allocated on-demand;