From 7cc00d9a832c42a330888aa5c11a2abad1bd5ac0 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Mon, 5 Aug 2013 23:49:53 +0200 Subject: [PATCH] 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 --- lib/ftp.c | 65 ++++++++++++++++++++++++------------------ lib/multi.c | 11 +++++-- lib/url.c | 10 ++++--- lib/url.h | 4 +-- lib/urldata.h | 2 +- tests/data/Makefile.am | 2 +- tests/data/test1233 | 46 ++++++++++++++++++++++++++++++ 7 files changed, 101 insertions(+), 39 deletions(-) create mode 100644 tests/data/test1233 diff --git a/lib/ftp.c b/lib/ftp.c index b6299ca06..9ebe3dc4a 100644 --- 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 774e80841..9653554f8 100644 --- 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 d2149abd8..7cec5bccd 100644 --- 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 a026e90e8..c0d9c38dc 100644 --- a/lib/url.h +++ b/lib/url.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2011, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2013, Daniel Stenberg, , 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 fb4f81c92..d597c6744 100644 --- 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 8e952115a..eaaf7eca2 100644 --- 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 index 000000000..caf0527f2 --- /dev/null +++ b/tests/data/test1233 @@ -0,0 +1,46 @@ + + + +FTP + + + +# Server-side + + +# Assuming there's nothing listening on port 1 +REPLY EPSV 229 Entering Passiv Mode (|||1|) + + +here are some bytes + + + +# Client-side + + +ftp + + +FTP failing to connect to EPSV port, switching to PASV + + +ftp://%HOSTIP:%FTPPORT/1233 + + + +# Verify data after the test has been "shot" + + +USER anonymous +PASS ftp@example.com +PWD +EPSV +PASV +TYPE I +SIZE 1233 +RETR 1233 +QUIT + + +