Do not send PASV until RETR or LIST

net/ftp/ftp_network_transaction.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/ftp/ftp_network_transaction.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram.h"
@@ skipping 206 matching lines @@
       last_error_(OK),
       system_type_(SYSTEM_TYPE_UNKNOWN),
       // Use image (binary) transfer by default. It should always work,
       // whereas the ascii transfer may damage binary data.
       data_type_(DATA_TYPE_IMAGE),
       resource_type_(RESOURCE_TYPE_UNKNOWN),
       use_epsv_(true),
       data_connection_port_(0),
       socket_factory_(socket_factory),
       next_state_(STATE_NONE),
       state_after_data_connect_complete_(STATE_CTRL_WRITE_SIZE) {}

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

This should probably be initialized to STATE_NONE now.

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

Done.

 
 FtpNetworkTransaction::~FtpNetworkTransaction() {
 }
 
 int FtpNetworkTransaction::Stop(int error) {
   if (command_sent_ == COMMAND_QUIT)
     return error;
 
   next_state_ = STATE_CTRL_WRITE_QUIT;
   last_error_ = error;
@@ skipping 100 matching lines @@
   ctrl_response_buffer_.reset(new FtpCtrlResponseBuffer(net_log_));
   read_data_buf_ = NULL;
   read_data_buf_len_ = 0;
   if (write_buf_.get())
     write_buf_->SetOffset(0);
   last_error_ = OK;
   data_connection_port_ = 0;
   ctrl_socket_.reset();
   data_socket_.reset();
   next_state_ = STATE_NONE;
   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) {
   DCHECK(rv != ERR_IO_PENDING);
   DCHECK(!user_callback_.is_null());
 
   // Since Run may result in Read being called, clear callback_ up front.
   CompletionCallback c = user_callback_;
   user_callback_.Reset();
   c.Run(rv);
@@ skipping 564 matching lines @@
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, command, COMMAND_TYPE);
 }
 
 int FtpNetworkTransaction::ProcessResponseTYPE(
     const FtpCtrlResponse& response) {
   switch (GetErrorClass(response.status_code)) {
     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);
     case ERROR_CLASS_TRANSIENT_ERROR:
       return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
     case ERROR_CLASS_PERMANENT_ERROR:
       return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
     default:
       NOTREACHED();
       return Stop(ERR_UNEXPECTED);
@@ skipping 102 matching lines @@
       // like "Not logged in" etc.
       if (response.status_code != 550 || resource_type_ == RESOURCE_TYPE_FILE)
         return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
 
       // It's possible that RETR failed because the path is a directory.
       resource_type_ = RESOURCE_TYPE_DIRECTORY;
 
       // We're going to try CWD next, but first send a PASV one more time,
       // because some FTP servers, including FileZilla, require that.
       // See http://crbug.com/25316.
       next_state_ = use_epsv_ ? STATE_CTRL_WRITE_EPSV : STATE_CTRL_WRITE_PASV;

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

I'm actually confused how this worked before this CL too, but this line looks
off. It doesn't set state_after_data_connect_. Looks like there was a test for
this case, but then it got removed in https://codereview.chromium.org/2813044
when the commands were reordered a bunch.

This line was originally added in https://codereview.chromium.org/293049 at
which point we apparently always transitioned to SIZE or CWD. I actually suspect
that the right thing to do is to remove this blog altogether now and just call
Stop(). (If removing it doesn't break tests, then we've clearly lost the test
cases for it.) I think this is from before 2813044 where our trace was (judging
from the tests in 2813044):

files:
USER
PASS
SYST
PWD
TYPE
PASV
SIZE
MDTM # modification time
RETR
QUIT

directories:
USER
PASS
SYST
PWD
TYPE
PASV
SIZE
MDTM # modification time
RETR # fails
PASV # this line, added by 293
CWD
LIST
QUIT

That is, we sniffed files vs directories based on RETR's failure. Then we
sniffed just based on SIZE. And then with http://codereview.chromium.org/5227004
we switched to using CWD, which is why we always follow SIZE with CWD now.

(In fact, I'm not sure it's even possible to get to ProcessReponseRETR without
resource_type_ being RESOURCE_TYPE_FILE anymore. I think 2813044 failed to fix
up a lot of the code and comments. Fixing all that in this CL is probably too
much for one change, but this RETR -> DIRECTORY state change is probably worth
eliminating.)

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

Done. Right, this part doesn't look correct. Thanks for catching it! The tests
are still passing.

       break;
     default:
       NOTREACHED();
       return Stop(ERR_UNEXPECTED);
   }
 
   // We should be sure about our resource type now. Otherwise we risk
   // an infinite loop (RETR can later send CWD, and CWD can later send RETR).
   DCHECK_NE(RESOURCE_TYPE_UNKNOWN, resource_type_);
 
   return OK;
 }
 
 // SIZE command
 int FtpNetworkTransaction::DoCtrlWriteSIZE() {
   std::string command = "SIZE " + GetRequestPathForFtpCommand(false);
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, command, COMMAND_SIZE);
 }
 
 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)
         return Stop(ERR_INVALID_RESPONSE);
       int64 size;
       if (!base::StringToInt64(response.lines[0], &size))
         return Stop(ERR_INVALID_RESPONSE);
       if (size < 0)
         return Stop(ERR_INVALID_RESPONSE);
 
       // A successful response to SIZE does not mean the resource is a file.
       // 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.
       if (resource_type_ == RESOURCE_TYPE_UNKNOWN &&
           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;
 }
 
 // CWD command
 int FtpNetworkTransaction::DoCtrlWriteCWD() {
   std::string command = "CWD " + GetRequestPathForFtpCommand(true);
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, command, COMMAND_CWD);
 }
 
 int FtpNetworkTransaction::ProcessResponseCWD(const FtpCtrlResponse& response) {
   // We should never issue CWD if we know the target resource is a file.
   DCHECK_NE(RESOURCE_TYPE_FILE, resource_type_);
 
   switch (GetErrorClass(response.status_code)) {
     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);
     case ERROR_CLASS_TRANSIENT_ERROR:
       // Some FTP servers send response 451 (not a valid CWD response according
       // to RFC 959) instead of 550.
       if (response.status_code == 451)
         return ProcessResponseCWDNotADirectory();
 
       return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
@@ skipping 16 matching lines @@
     // says it's not true. The most probable interpretation is that it
     // doesn't exist (with FTP we can't be sure).
     return Stop(ERR_FILE_NOT_FOUND);
   }
 
   // We are here because SIZE failed and we are not sure what the resource
   // type is. It could still be file, and SIZE could fail because of
   // 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;
 }
 
 // LIST command
 int FtpNetworkTransaction::DoCtrlWriteLIST() {
   // Use the -l option for mod_ftp configured in LISTIsNLST mode: the option
   // forces LIST output instead of NLST (which would be ambiguous for us
   // to parse).
   std::string command("LIST -l");
   if (system_type_ == SYSTEM_TYPE_VMS)
@@ skipping 198 matching lines @@
   if (!had_error_type[type]) {
     had_error_type[type] = true;
     UMA_HISTOGRAM_ENUMERATION("Net.FtpDataConnectionErrorHappened",
         type, NUM_OF_NET_ERROR_TYPES);
   }
   UMA_HISTOGRAM_ENUMERATION("Net.FtpDataConnectionErrorCount",
       type, NUM_OF_NET_ERROR_TYPES);
 }
 
 }  // namespace net