You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
395 lines
14 KiB
395 lines
14 KiB
From 3f411052825386a95d039435eb139a63859c3c73 Mon Sep 17 00:00:00 2001 |
|
From: Daniel Stenberg <daniel@haxx.se> |
|
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 |
|
|
|
[upstream commit 7cc00d9a832c42a330888aa5c11a2abad1bd5ac0] |
|
|
|
Signed-off-by: Kamil Dudka <kdudka@redhat.com> |
|
--- |
|
lib/ftp.c | 64 ++++++++++++++++++++++++++++------------------- |
|
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, 102 insertions(+), 37 deletions(-) |
|
create mode 100644 tests/data/test1233 |
|
|
|
diff --git a/lib/ftp.c b/lib/ftp.c |
|
index 469b887..4501116 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); |
|
@@ -1794,15 +1794,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, "PASV", NULL); |
|
conn->proto.ftpc.count1++; |
|
- /* remain in the FTP_PASV state */ |
|
+ /* remain in/go to the FTP_PASV state */ |
|
+ state(conn, FTP_PASV); |
|
return result; |
|
} |
|
|
|
@@ -1931,15 +1931,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, "PASV", NULL); |
|
- ftpc->count1++; |
|
- /* remain in the FTP_PASV state */ |
|
- return result; |
|
+ return ftp_epsv_disable(conn); |
|
} |
|
else { |
|
failf(data, "Bad PASV/EPSV response: %03d", ftpcode); |
|
@@ -2018,14 +2010,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: |
|
@@ -2077,8 +2072,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 */ |
|
|
|
@@ -3664,20 +3658,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) { |
|
@@ -3694,14 +3691,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 */ |
|
@@ -3712,7 +3717,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) { |
|
@@ -3735,6 +3740,9 @@ static CURLcode ftp_do_more(struct connectdata *conn, bool *complete) |
|
|
|
if(result) |
|
return result; |
|
+ |
|
+ *completep = 1; /* this state is now complete when the server has |
|
+ connected back to us */ |
|
} |
|
} |
|
else if(data->set.upload) { |
|
@@ -3742,7 +3750,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 */ |
|
@@ -3770,7 +3779,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; |
|
} |
|
@@ -3782,7 +3792,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)); |
|
} |
|
|
|
@@ -3825,7 +3835,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")); |
|
@@ -4445,7 +4457,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 706df23..9a8e68e 100644 |
|
--- a/lib/multi.c |
|
+++ b/lib/multi.c |
|
@@ -906,6 +906,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->easy_handle)) |
|
return CURLM_BAD_EASY_HANDLE; |
|
@@ -1323,13 +1324,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 b269027..52f7e27 100644 |
|
--- a/lib/url.c |
|
+++ b/lib/url.c |
|
@@ -5394,18 +5394,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 |
|
--- 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 7a275da..2be467b 100644 |
|
--- a/lib/urldata.h |
|
+++ b/lib/urldata.h |
|
@@ -550,7 +550,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 3e8dae0..3f6a047 100644 |
|
--- a/tests/data/Makefile.am |
|
+++ b/tests/data/Makefile.am |
|
@@ -78,7 +78,7 @@ test1118 test1119 test1120 test1121 test1122 test1123 test1124 test1125 \ |
|
test1126 test1127 test1128 test1129 test1130 test1131 test1132 test1133 \ |
|
test1200 test1201 test1202 test1203 test1204 test1205 test1206 test1207 \ |
|
test1208 test1209 test1210 test1211 test1216 test1218 \ |
|
-test1220 test1221 test1222 test1223 \ |
|
+test1220 test1221 test1222 test1223 test1233 \ |
|
test1300 test1301 test1302 test1303 test1304 test1305 \ |
|
test1306 test1307 test1308 test1309 test1310 test1311 test1312 test1313 \ |
|
test1314 test1315 test1316 test1317 test1318 test1319 test1320 test1321 \ |
|
diff --git a/tests/data/test1233 b/tests/data/test1233 |
|
new file mode 100644 |
|
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 |
|
+PASS ftp@example.com |
|
+PWD |
|
+EPSV |
|
+PASV |
|
+TYPE I |
|
+SIZE 1233 |
|
+RETR 1233 |
|
+QUIT |
|
+</protocol> |
|
+</verify> |
|
+</testcase> |
|
-- |
|
1.7.1 |
|
|
|
|