- Linus Nielsen Feltzing reported and helped me repeat and fix a problem with

  FTP with the multi interface: when a transfer fails, like when aborted by a
  write callback, the control connection was wrongly closed and thus not
  re-used properly.

  This change is also an attempt to cleanup the code somewhat in this area, as
  now the FTP code attempts to keep (better) track on pending responses
  necessary to get read in ftp_done().

5 files changed, 43 insertions, 21 deletions

diff --git a/CHANGES b/CHANGES
index 9f30f0dc4..8c480ade9 100644
--- a/CHANGES
+++ b/CHANGES
@@ -6,6 +6,16 @@
 
                                   Changelog
 
+Daniel Stenberg (20 Feb 2009)
+- Linus Nielsen Feltzing reported and helped me repeat and fix a problem with
+  FTP with the multi interface: when a transfer fails, like when aborted by a
+  write callback, the control connection was wrongly closed and thus not
+  re-used properly.
+
+  This change is also an attempt to cleanup the code somewhat in this area, as
+  now the FTP code attempts to keep (better) track on pending responses
+  necessary to get read in ftp_done().
+
 Daniel Stenberg (19 Feb 2009)
 - Patrik Thunstrom reported a problem and helped me repeat it. It turned out 
   libcurl did a superfluous 1000ms wait when doing SFTP downloads!
diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index d6da34205..972157474 100644
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -47,6 +47,6 @@ advice from friends like these:
  Lisa Xu, Daniel Fandrich, Craig A West, Alexey Borzov, Sharad Gupta,
  Peter Sylvester, Chad Monroe, Markus Moeller, Yang Tse, Scott Cantor,
  Patrick Scott, Hidemoto Nakada, Jocelyn Jaubert, Andre Guibert de Bruet,
- Kamil Dudka, Patrik Thunstrom
+ Kamil Dudka, Patrik Thunstrom, Linus Nielsen Feltzing
 
         Thanks! (and sorry if I forgot to mention someone)
diff --git a/lib/ftp.c b/lib/ftp.c
index cf2d4db6a..426f4af76 100644
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -363,6 +363,7 @@ static void ftp_respinit(struct connectdata *conn)
   struct ftp_conn *ftpc = &conn->proto.ftpc;
   ftpc->nread_resp = 0;
   ftpc->linestart_resp = conn->data->state.buffer;
+  ftpc->pending_resp = TRUE;
 }
 
 /* macro to check for a three-digit ftp status code at the start of the
@@ -590,6 +591,8 @@ static CURLcode ftp_readresp(curl_socket_t sockfd,
   /* store the latest code for later retrieval */
   conn->data->info.httpcode=code;
 
+  ftpc->pending_resp = FALSE;
+
   return result;
 }
 
@@ -715,6 +718,8 @@ CURLcode Curl_GetFTPResponse(ssize_t *nreadp, /* return number of bytes read */
 
   } /* while there's buffer left and loop is requested */
 
+  ftpc->pending_resp = FALSE;
+
   return result;
 }
 
@@ -2305,6 +2310,8 @@ static CURLcode ftp_state_stor_resp(struct connectdata *conn,
                                SECONDARYSOCKET, ftp->bytecountp);
   state(conn, FTP_STOP);
 
+  conn->proto.ftpc.pending_resp = TRUE; /* we expect a server response more */
+
   return result;
 }
 
@@ -2417,6 +2424,7 @@ static CURLcode ftp_state_get_resp(struct connectdata *conn,
     if(result)
       return result;
 
+    conn->proto.ftpc.pending_resp = TRUE; /* we expect a server response more */
     state(conn, FTP_STOP);
   }
   else {
@@ -3161,6 +3169,7 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status,
   case CURLE_REMOTE_ACCESS_DENIED:
   case CURLE_FILESIZE_EXCEEDED:
   case CURLE_REMOTE_FILE_NOT_FOUND:
+  case CURLE_WRITE_ERROR:
     /* the connection stays alive fine even though this happened */
     /* fall-through */
   case CURLE_OK: /* doesn't affect the control connection's status */
@@ -3239,7 +3248,8 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status,
     conn->sock[SECONDARYSOCKET] = CURL_SOCKET_BAD;
   }
 
-  if((ftp->transfer == FTPTRANSFER_BODY) && !status && !premature) {
+  if((ftp->transfer == FTPTRANSFER_BODY) && ftpc->ctl_valid &&
+     ftpc->pending_resp && !premature) {
     /*
      * Let's see what the server says about the transfer we just performed,
      * but lower the timeout as sometimes this connection has died while the
diff --git a/lib/multi.c b/lib/multi.c
index 15339a3d6..2ce023331 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -1237,14 +1237,6 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
       break;
 
     case CURLM_STATE_WAITPERFORM:
-#ifdef CURLDEBUG
-      infof(easy->easy_handle, "Conn %d recv pipe %d inuse %d athead %d\n",
-            easy->easy_conn->connectindex,
-            easy->easy_conn->recv_pipe->size,
-            easy->easy_conn->readchannel_inuse,
-            isHandleAtHead(easy->easy_handle,
-                           easy->easy_conn->recv_pipe));
-#endif
       /* Wait for our turn to PERFORM */
       if(!easy->easy_conn->readchannel_inuse &&
          isHandleAtHead(easy->easy_handle,
@@ -1254,6 +1246,16 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         multistate(easy, CURLM_STATE_PERFORM);
         result = CURLM_CALL_MULTI_PERFORM;
       }
+#ifdef CURLDEBUG
+      else {
+        infof(easy->easy_handle, "Conn %d recv pipe %d inuse %d athead %d\n",
+              easy->easy_conn->connectindex,
+              easy->easy_conn->recv_pipe->size,
+              easy->easy_conn->readchannel_inuse,
+              isHandleAtHead(easy->easy_handle,
+                             easy->easy_conn->recv_pipe));
+      }
+#endif
       break;
 
     case CURLM_STATE_TOOFAST: /* limit-rate exceeded in either direction */
@@ -1302,18 +1304,14 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
 
       if(easy->result)  {
         /* The transfer phase returned error, we mark the connection to get
-         * closed to prevent being re-used. This is because we can't
-         * possibly know if the connection is in a good shape or not now. */
-        easy->easy_conn->bits.close = TRUE;
-        Curl_removeHandleFromPipeline(easy->easy_handle,
-                                      easy->easy_conn->recv_pipe);
+         * closed to prevent being re-used. This is because we can't possibly
+         * know if the connection is in a good shape or not now.  Unless it is
+         * a protocol which uses two "channels" like FTP, as then the error
+         * happened in the data connection.
+         */
+        if(!(easy->easy_conn->protocol & PROT_DUALCHANNEL))
+          easy->easy_conn->bits.close = TRUE;
 
-        if(CURL_SOCKET_BAD != easy->easy_conn->sock[SECONDARYSOCKET]) {
-          /* if we failed anywhere, we must clean up the secondary socket if
-             it was used */
-          sclose(easy->easy_conn->sock[SECONDARYSOCKET]);
-          easy->easy_conn->sock[SECONDARYSOCKET] = CURL_SOCKET_BAD;
-        }
         Curl_posttransfer(easy->easy_handle);
         Curl_done(&easy->easy_conn, easy->result, FALSE);
       }
diff --git a/lib/urldata.h b/lib/urldata.h
index d1851066e..6969f1ebb 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -438,6 +438,9 @@ struct ftp_conn {
   size_t nread_resp; /* number of bytes currently read of a server response */
   char *linestart_resp; /* line start pointer for the FTP server response
                            reader function */
+  bool pending_resp;  /* set TRUE when a server response is pending or in
+                         progress, and is cleared once the last response is
+                         read */
 
   int count1; /* general purpose counter for the state machine */
   int count2; /* general purpose counter for the state machine */
@@ -904,6 +907,7 @@ struct connectdata {
 
 #define PROT_CLOSEACTION PROT_FTP /* these ones need action before socket
                                      close */
+#define PROT_DUALCHANNEL PROT_FTP /* these protocols use two connections */
 
   /* 'dns_entry' is the particular host we use. This points to an entry in the
      DNS cache and it will not get pruned while locked. It gets unlocked in