- 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().

3 files changed, 32 insertions, 20 deletions

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