FTP: when EPSV gets a 229 but fails to connect, retry with PASV

This is a regression as this logic used to work. It isn't clear when it
broke, but I'm assuming in 7.28.0 when we went all-multi internally.

This likely never worked with the multi interface. As the failed
connection is detected once the multi state has reached DO_MORE, the
Curl_do_more() function was now expanded somewhat so that the
ftp_do_more() function can request to go "back" to the previous state
when it makes another attempt - using PASV.

Added test case 1233 to verify this fix. It has the little issue that it
assumes no service is listening/accepting connections on port 1...

Reported-by: byte_bucket in the #curl IRC channel

diff --git a/lib/ftp.c b/lib/ftp.c
index b6299ca..9ebe3dc 100644 (file)
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -136,7 +136,7 @@ static CURLcode ftp_done(struct connectdata *conn,
                          CURLcode, bool premature);
 static CURLcode ftp_connect(struct connectdata *conn, bool *done);
 static CURLcode ftp_disconnect(struct connectdata *conn, bool dead_connection);
-static CURLcode ftp_do_more(struct connectdata *conn, bool *completed);
+static CURLcode ftp_do_more(struct connectdata *conn, int *completed);
 static CURLcode ftp_multi_statemach(struct connectdata *conn, bool *done);
 static int ftp_getsock(struct connectdata *conn, curl_socket_t *socks,
                        int numsocks);
@@ -1799,15 +1799,15 @@ static CURLcode ftp_state_quote(struct connectdata *conn,
 static CURLcode ftp_epsv_disable(struct connectdata *conn)
 {
   CURLcode result = CURLE_OK;
-  infof(conn->data, "got positive EPSV response, but can't connect. "
-        "Disabling EPSV\n");
+  infof(conn->data, "Failed EPSV attempt. Disabling EPSV\n");
   /* disable it for next transfer */
   conn->bits.ftp_use_epsv = FALSE;
   conn->data->state.errorbuf = FALSE; /* allow error message to get
                                          rewritten */
   PPSENDF(&conn->proto.ftpc.pp, "%s", "PASV");
   conn->proto.ftpc.count1++;
-  /* remain in the FTP_PASV state */
+  /* remain in/go to the FTP_PASV state */
+  state(conn, FTP_PASV);
   return result;
 }
 
@@ -1936,15 +1936,7 @@ static CURLcode ftp_state_pasv_resp(struct connectdata *conn,
   }
   else if(ftpc->count1 == 0) {
     /* EPSV failed, move on to PASV */
-
-    /* disable it for next transfer */
-    conn->bits.ftp_use_epsv = FALSE;
-    infof(data, "disabling EPSV usage\n");
-
-    PPSENDF(&ftpc->pp, "%s", "PASV");
-    ftpc->count1++;
-    /* remain in the FTP_PASV state */
-    return result;
+    return ftp_epsv_disable(conn);
   }
   else {
     failf(data, "Bad PASV/EPSV response: %03d", ftpcode);
@@ -2021,14 +2013,17 @@ static CURLcode ftp_state_pasv_resp(struct connectdata *conn,
   case CURLPROXY_SOCKS5_HOSTNAME:
     result = Curl_SOCKS5(conn->proxyuser, conn->proxypasswd, newhost, newport,
                          SECONDARYSOCKET, conn);
+    connected = TRUE;
     break;
   case CURLPROXY_SOCKS4:
     result = Curl_SOCKS4(conn->proxyuser, newhost, newport,
                          SECONDARYSOCKET, conn, FALSE);
+    connected = TRUE;
     break;
   case CURLPROXY_SOCKS4A:
     result = Curl_SOCKS4(conn->proxyuser, newhost, newport,
                          SECONDARYSOCKET, conn, TRUE);
+    connected = TRUE;
     break;
   case CURLPROXY_HTTP:
   case CURLPROXY_HTTP_1_0:
@@ -2080,8 +2075,7 @@ static CURLcode ftp_state_pasv_resp(struct connectdata *conn,
     }
   }
 
-  conn->bits.tcpconnect[SECONDARYSOCKET] = TRUE;
-
+  conn->bits.tcpconnect[SECONDARYSOCKET] = connected;
   conn->bits.do_more = TRUE;
   state(conn, FTP_STOP); /* this phase is completed */
 
@@ -3677,20 +3671,23 @@ static CURLcode ftp_range(struct connectdata *conn)
  *
  * This function shall be called when the second FTP (data) connection is
  * connected.
+ *
+ * 'complete' can return 0 for incomplete, 1 for done and -1 for go back
+ * (which basically is only for when PASV is being sent to retry a failed
+ * EPSV).
  */
 
-static CURLcode ftp_do_more(struct connectdata *conn, bool *complete)
+static CURLcode ftp_do_more(struct connectdata *conn, int *completep)
 {
   struct SessionHandle *data=conn->data;
   struct ftp_conn *ftpc = &conn->proto.ftpc;
   CURLcode result = CURLE_OK;
   bool connected = FALSE;
+  bool complete = FALSE;
 
   /* the ftp struct is inited in ftp_connect() */
   struct FTP *ftp = data->state.proto.ftp;
 
-  *complete = FALSE;
-
   /* if the second connection isn't done yet, wait for it */
   if(!conn->bits.tcpconnect[SECONDARYSOCKET]) {
     if(conn->tunnel_state[SECONDARYSOCKET] == TUNNEL_CONNECT) {
@@ -3707,14 +3704,22 @@ static CURLcode ftp_do_more(struct connectdata *conn, bool *complete)
     if(connected) {
       DEBUGF(infof(data, "DO-MORE connected phase starts\n"));
     }
-    else
+    else {
+      if(result && (ftpc->count1 == 0)) {
+        *completep = -1; /* go back to DOING please */
+        /* this is a EPSV connect failing, try PASV instead */
+        return ftp_epsv_disable(conn);
+      }
       return result;
+    }
   }
 
   if(ftpc->state) {
     /* already in a state so skip the intial commands.
        They are only done to kickstart the do_more state */
-    result = ftp_multi_statemach(conn, complete);
+    result = ftp_multi_statemach(conn, &complete);
+
+    *completep = (int)complete;
 
     /* if we got an error or if we don't wait for a data connection return
        immediately */
@@ -3725,7 +3730,7 @@ static CURLcode ftp_do_more(struct connectdata *conn, bool *complete)
       /* if we reach the end of the FTP state machine here, *complete will be
          TRUE but so is ftpc->wait_data_conn, which says we need to wait for
          the data connection and therefore we're not actually complete */
-      *complete = FALSE;
+      *completep = 0;
   }
 
   if(ftp->transfer <= FTPTRANSFER_INFO) {
@@ -3749,8 +3754,8 @@ static CURLcode ftp_do_more(struct connectdata *conn, bool *complete)
         if(result)
           return result;
 
-        *complete = TRUE; /* this state is now complete when the server has
-                             connected back to us */
+        *completep = 1; /* this state is now complete when the server has
+                           connected back to us */
       }
     }
     else if(data->set.upload) {
@@ -3758,7 +3763,8 @@ static CURLcode ftp_do_more(struct connectdata *conn, bool *complete)
       if(result)
         return result;
 
-      result = ftp_multi_statemach(conn, complete);
+      result = ftp_multi_statemach(conn, &complete);
+      *completep = (int)complete;
     }
     else {
       /* download */
@@ -3786,7 +3792,8 @@ static CURLcode ftp_do_more(struct connectdata *conn, bool *complete)
           return result;
       }
 
-      result = ftp_multi_statemach(conn, complete);
+      result = ftp_multi_statemach(conn, &complete);
+      *completep = (int)complete;
     }
     return result;
   }
@@ -3798,7 +3805,7 @@ static CURLcode ftp_do_more(struct connectdata *conn, bool *complete)
 
   if(!ftpc->wait_data_conn) {
     /* no waiting for the data connection so this is now complete */
-    *complete = TRUE;
+    *completep = 1;
     DEBUGF(infof(data, "DO-MORE phase ends with %d\n", (int)result));
   }
 
@@ -3841,7 +3848,9 @@ CURLcode ftp_perform(struct connectdata *conn,
   /* run the state-machine */
   result = ftp_multi_statemach(conn, dophase_done);
 
-  *connected = conn->bits.tcpconnect[FIRSTSOCKET];
+  *connected = conn->bits.tcpconnect[SECONDARYSOCKET];
+
+  infof(conn->data, "ftp_perform ends with SECONDARY: %d\n", *connected);
 
   if(*dophase_done)
     DEBUGF(infof(conn->data, "DO phase is complete1\n"));
@@ -4469,7 +4478,7 @@ static CURLcode ftp_dophase_done(struct connectdata *conn,
   struct ftp_conn *ftpc = &conn->proto.ftpc;
 
   if(connected) {
-    bool completed;
+    int completed;
     CURLcode result = ftp_do_more(conn, &completed);
 
     if(result) {

diff --git a/lib/multi.c b/lib/multi.c
index 774e808..9653554 100644 (file)
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -929,6 +929,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
   struct SingleRequest *k;
   struct SessionHandle *data;
   long timeout_ms;
+  int control;
 
   if(!GOOD_EASY_HANDLE(easy))
     return CURLM_BAD_EASY_HANDLE;
@@ -1359,13 +1360,17 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
       /*
        * When we are connected, DO MORE and then go DO_DONE
        */
-      easy->result = Curl_do_more(easy->easy_conn, &dophase_done);
+      easy->result = Curl_do_more(easy->easy_conn, &control);
 
       /* No need to remove this handle from the send pipeline here since that
          is done in Curl_done() */
       if(CURLE_OK == easy->result) {
-        if(dophase_done) {
-          multistate(easy, CURLM_STATE_DO_DONE);
+        if(control) {
+          /* if positive, advance to DO_DONE
+             if negative, go back to DOING */
+          multistate(easy, control==1?
+                     CURLM_STATE_DO_DONE:
+                     CURLM_STATE_DOING);
           result = CURLM_CALL_MULTI_PERFORM;
         }
         else

diff --git a/lib/url.c b/lib/url.c
index d2149ab..7cec5bc 100644 (file)
--- a/lib/url.c
+++ b/lib/url.c
@@ -5841,18 +5841,20 @@ CURLcode Curl_do(struct connectdata **connp, bool *done)
  *
  * TODO: A future libcurl should be able to work away this state.
  *
+ * 'complete' can return 0 for incomplete, 1 for done and -1 for go back to
+ * DOING state there's more work to do!
  */
 
-CURLcode Curl_do_more(struct connectdata *conn, bool *completed)
+CURLcode Curl_do_more(struct connectdata *conn, int *complete)
 {
   CURLcode result=CURLE_OK;
 
-  *completed = FALSE;
+  *complete = 0;
 
   if(conn->handler->do_more)
-    result = conn->handler->do_more(conn, completed);
+    result = conn->handler->do_more(conn, complete);
 
-  if(!result && *completed)
+  if(!result && (*complete == 1))
     /* do_complete must be called after the protocol-specific DO function */
     do_complete(conn);
 

diff --git a/lib/url.h b/lib/url.h
index a026e90..c0d9c38 100644 (file)
--- a/lib/url.h
+++ b/lib/url.h
@@ -7,7 +7,7 @@
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 1998 - 2011, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2013, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
@@ -37,7 +37,7 @@ CURLcode Curl_close(struct SessionHandle *data); /* opposite of curl_open() */
 CURLcode Curl_connect(struct SessionHandle *, struct connectdata **,
                       bool *async, bool *protocol_connect);
 CURLcode Curl_do(struct connectdata **, bool *done);
-CURLcode Curl_do_more(struct connectdata *, bool *completed);
+CURLcode Curl_do_more(struct connectdata *, int *completed);
 CURLcode Curl_done(struct connectdata **, CURLcode, bool premature);
 CURLcode Curl_disconnect(struct connectdata *, bool dead_connection);
 CURLcode Curl_protocol_connect(struct connectdata *conn, bool *done);

diff --git a/lib/urldata.h b/lib/urldata.h
index fb4f81c..d597c67 100644 (file)
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -577,7 +577,7 @@ struct Curl_async {
 /* These function pointer types are here only to allow easier typecasting
    within the source when we need to cast between data pointers (such as NULL)
    and function pointers. */
-typedef CURLcode (*Curl_do_more_func)(struct connectdata *, bool *);
+typedef CURLcode (*Curl_do_more_func)(struct connectdata *, int *);
 typedef CURLcode (*Curl_done_func)(struct connectdata *, CURLcode, bool);
 
 

diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 8e95211..eaaf7ec 100644 (file)
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -93,7 +93,7 @@ test1200 test1201 test1202 test1203 test1204 test1205 test1206 test1207 \
 test1208 test1209 test1210 test1211 test1212 test1213 test1214 test1215 \
 test1216 test1217 test1218 test1219 \
 test1220 test1221 test1222 test1223 test1224 test1225 test1226 test1227 \
-test1228 test1229 test1230 test1231 test1232 \
+test1228 test1229 test1230 test1231 test1232 test1233 \
 \
 test1300 test1301 test1302 test1303 test1304 test1305 test1306 test1307 \
 test1308 test1309 test1310 test1311 test1312 test1313 test1314 test1315 \

diff --git a/tests/data/test1233 b/tests/data/test1233
new file mode 100644 (file)
index 0000000..caf0527
--- /dev/null
+++ b/tests/data/test1233
@@ -0,0 +1,46 @@
+<testcase>
+<info>
+<keywords>
+FTP
+</keywords>
+</info>
+
+# Server-side
+<reply>
+<servercmd>
+# Assuming there's nothing listening on port 1
+REPLY EPSV 229 Entering Passiv Mode (|||1|)
+</servercmd>
+<data>
+here are some bytes
+</data>
+</reply>
+
+# Client-side
+<client>
+<server>
+ftp
+</server>
+ <name>
+FTP failing to connect to EPSV port, switching to PASV
+ </name>
+ <command>
+ftp://%HOSTIP:%FTPPORT/1233
+</command>
+</client>
+
+# Verify data after the test has been "shot"
+<verify>
+<protocol>
+USER anonymous\r
+PASS ftp@example.com\r
+PWD\r
+EPSV\r
+PASV\r
+TYPE I\r
+SIZE 1233\r
+RETR 1233\r
+QUIT\r
+</protocol>
+</verify>
+</testcase>