Do not send PASV until RETR or LIST

net/ftp/ftp_network_transaction.cc

Index: net/ftp/ftp_network_transaction.cc
diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index 17d5754b6c01fb9f98c3ae30891bf6f43b64703d..b8bd2ffe46804ffec60f6b3f693cf39fa2c0d0d5 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -348,18 +348,15 @@ void FtpNetworkTransaction::ResetStateForRestart() {
   state_after_data_connect_complete_ = STATE_CTRL_WRITE_SIZE;

[davidben, 2015/01/27 19:58:59]

This should be STATE_NONE now.

[xunjieli, 2015/01/27 20:55:28]

Done.

 }
 
-void FtpNetworkTransaction::ResetDataConnectionAfterError(State next_state) {
-  // The server _might_ have reset the data connection
-  // (see RFC 959 3.2. ESTABLISHING DATA CONNECTIONS:
-  // "The server MUST close the data connection under the following
-  // conditions:
-  // ...
-  // 5. An irrecoverable error condition occurs.")
-  //
-  // It is ambiguous what an irrecoverable error condition is,
-  // so we take no chances.
-  state_after_data_connect_complete_ = next_state;
-  next_state_ = use_epsv_ ? STATE_CTRL_WRITE_EPSV : STATE_CTRL_WRITE_PASV;
+void FtpNetworkTransaction::EstablishDataConnection(State state_after_connect) {
+  // Do data connect if the state that follows data connect is RETR or LIST.
+  if (state_after_connect == STATE_CTRL_WRITE_RETR ||
+      state_after_connect == STATE_CTRL_WRITE_LIST) {
+    state_after_data_connect_complete_ = state_after_connect;
+    next_state_ = use_epsv_ ? STATE_CTRL_WRITE_EPSV : STATE_CTRL_WRITE_PASV;

[davidben, 2015/01/27 19:58:59]

Nit: I'd probably just do rather than check + NOTREACHED. (It's pretty easy to
visually verify that EstablishDataConnection only gets called with those two
anyway.)

DCHECK(state_after_connect == STATE_CTRL_WRITE_RETR || state_after_connect ==
STATE_CTRL_WRITE_LIST); at the top.

[xunjieli, 2015/01/27 20:55:28]

Done. Good idea. thanks

+  } else {
+    NOTREACHED();
+  }
 }
 
 void FtpNetworkTransaction::DoCallback(int rv) {
@@ -944,7 +941,7 @@ int FtpNetworkTransaction::ProcessResponseTYPE(
     case ERROR_CLASS_INITIATED:
       return Stop(ERR_INVALID_RESPONSE);
     case ERROR_CLASS_OK:
-      next_state_ = use_epsv_ ? STATE_CTRL_WRITE_EPSV : STATE_CTRL_WRITE_PASV;
+      next_state_ = STATE_CTRL_WRITE_SIZE;
       break;
     case ERROR_CLASS_INFO_NEEDED:
       return Stop(ERR_INVALID_RESPONSE);
@@ -1090,15 +1087,8 @@ int FtpNetworkTransaction::DoCtrlWriteSIZE() {
 
 int FtpNetworkTransaction::ProcessResponseSIZE(
     const FtpCtrlResponse& response) {
-  State state_after_size;
-  if (resource_type_ == RESOURCE_TYPE_FILE)
-    state_after_size = STATE_CTRL_WRITE_RETR;
-  else
-    state_after_size = STATE_CTRL_WRITE_CWD;
-
   switch (GetErrorClass(response.status_code)) {
     case ERROR_CLASS_INITIATED:
-      next_state_ = state_after_size;
       break;
     case ERROR_CLASS_OK:
       if (response.lines.size() != 1)
@@ -1113,14 +1103,10 @@ int FtpNetworkTransaction::ProcessResponseSIZE(
       // Some FTP servers (for example, the qnx one) send a SIZE even for
       // directories.
       response_.expected_content_size = size;
-
-      next_state_ = state_after_size;
       break;
     case ERROR_CLASS_INFO_NEEDED:
-      next_state_ = state_after_size;
       break;
     case ERROR_CLASS_TRANSIENT_ERROR:
-      ResetDataConnectionAfterError(state_after_size);
       break;
     case ERROR_CLASS_PERMANENT_ERROR:
       // It's possible that SIZE failed because the path is a directory.
@@ -1128,14 +1114,15 @@ int FtpNetworkTransaction::ProcessResponseSIZE(
           response.status_code != 550) {
         return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
       }
-
-      ResetDataConnectionAfterError(state_after_size);
       break;
     default:
       NOTREACHED();
       return Stop(ERR_UNEXPECTED);
   }
-
+  if (resource_type_ == RESOURCE_TYPE_FILE)
+    EstablishDataConnection(STATE_CTRL_WRITE_RETR);
+  else
+    next_state_ = STATE_CTRL_WRITE_CWD;
   return OK;
 }
 
@@ -1154,7 +1141,7 @@ int FtpNetworkTransaction::ProcessResponseCWD(const FtpCtrlResponse& response) {
     case ERROR_CLASS_INITIATED:
       return Stop(ERR_INVALID_RESPONSE);
     case ERROR_CLASS_OK:
-      next_state_ = STATE_CTRL_WRITE_LIST;
+      EstablishDataConnection(STATE_CTRL_WRITE_LIST);
       break;
     case ERROR_CLASS_INFO_NEEDED:
       return Stop(ERR_INVALID_RESPONSE);
@@ -1191,7 +1178,7 @@ int FtpNetworkTransaction::ProcessResponseCWDNotADirectory() {
   // an access error (http://crbug.com/56734). Try RETR just to be sure.
   resource_type_ = RESOURCE_TYPE_FILE;
 
-  ResetDataConnectionAfterError(STATE_CTRL_WRITE_RETR);
+  EstablishDataConnection(STATE_CTRL_WRITE_RETR);
   return OK;
 }