Correctly handle multiple control response lines in new FTP transaction

After this change it should correctly handle all cases of multiple 230 welcome responses. It also fixes some other related bugs.

TEST=Covered by net_unittests.
BUG=none

Review URL: http://codereview.chromium.org/149211

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@20148 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 65 insertions, 6 deletions

diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index 5b3ff67..980e0ab 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -158,7 +158,11 @@ int FtpNetworkTransaction::ParseCtrlResponse(int* cut_pos) {
           return ERR_INVALID_RESPONSE;
         ctrl_responses_.push(ResponseLine(status_code, status_text));
         *cut_pos = i + 1;
+
+        // Prepare to handle the next line.
         scan_state = CODE;
+        status_code = 0;
+        status_text = "";
         break;
       default:
         NOTREACHED();
@@ -172,7 +176,6 @@ int FtpNetworkTransaction::ParseCtrlResponse(int* cut_pos) {
 // Used to prepare and send FTP commad.
 int FtpNetworkTransaction::SendFtpCommand(const std::string& command,
                                           Command cmd) {
-  response_message_buf_len_ = 0;
   command_sent_ = cmd;
   DLOG(INFO) << " >> " << command;
   const char* buf = command.c_str();
@@ -199,8 +202,29 @@ int FtpNetworkTransaction::ProcessCtrlResponses() {
     return rv;
   }
 
-  // TODO(phajdan.jr): Correctly handle multiple code 230 response lines after
-  // PASS command.
+  // Eat multiple 230 lines after PASS.
+  if (command_sent_ == COMMAND_PASS) {
+    while (ctrl_responses_.size() > 1) {
+      if (ctrl_responses_.front().code != 230)
+        break;
+      ctrl_responses_.pop();
+    }
+  }
+
+  // Make sure there are no 230's when we want to process SYST response.
+  if (command_sent_ == COMMAND_SYST) {
+    while (!ctrl_responses_.empty()) {
+      if (ctrl_responses_.front().code != 230)
+        break;
+      ctrl_responses_.pop();
+    }
+    if (ctrl_responses_.empty()) {
+      // Read more from control socket.
+      next_state_ = STATE_CTRL_READ;
+      return rv;
+    }
+  }
+
   if (ctrl_responses_.size() != 1)
     return Stop(ERR_INVALID_RESPONSE);
 
diff --git a/net/ftp/ftp_network_transaction_unittest.cc b/net/ftp/ftp_network_transaction_unittest.cc
index 51cf77b..c81fc0f 100644
--- a/net/ftp/ftp_network_transaction_unittest.cc
+++ b/net/ftp/ftp_network_transaction_unittest.cc
@@ -44,7 +44,9 @@ class FtpMockControlSocket : public DynamicMockSocket {
     QUIT
   };
 
-  FtpMockControlSocket() : failure_injection_state_(NONE) {
+  FtpMockControlSocket()
+      : failure_injection_state_(NONE),
+        multiline_welcome_(false) {
     Init();
   }
 
@@ -56,8 +58,12 @@ class FtpMockControlSocket : public DynamicMockSocket {
         return Verify("USER anonymous\r\n", data, PRE_PASSWD,
                       "331 Password needed\r\n");
       case PRE_PASSWD:
-        return Verify("PASS chrome@example.com\r\n", data, PRE_SYST,
-                      "230 Welcome\r\n");
+        {
+          const char* response_one = "230 Welcome\r\n";
+          const char* response_multi = "230 One\r\n230 Two\r\n230 Three\r\n";
+          return Verify("PASS chrome@example.com\r\n", data, PRE_SYST,
+                        multiline_welcome_ ? response_multi : response_one);
+        }
       case PRE_SYST:
         return Verify("SYST\r\n", data, PRE_PWD, "215 UNIX\r\n");
       case PRE_PWD:
@@ -95,6 +101,10 @@ class FtpMockControlSocket : public DynamicMockSocket {
     Init();
   }
 
+  void set_multiline_welcome(bool multiline) {
+    multiline_welcome_ = multiline;
+  }
+
  protected:
   void Init() {
     state_ = PRE_USER;
@@ -130,6 +140,9 @@ class FtpMockControlSocket : public DynamicMockSocket {
   State failure_injection_next_state_;
   const char* fault_response_;
 
+  // If true, we will send multiple 230 lines as response after PASS.
+  bool multiline_welcome_;
+
   DISALLOW_COPY_AND_ASSIGN(FtpMockControlSocket);
 };
 
@@ -293,6 +306,12 @@ TEST_F(FtpNetworkTransactionTest, DirectoryTransaction) {
   ExecuteTransaction(&ctrl_socket, "ftp://host", OK);
 }
 
+TEST_F(FtpNetworkTransactionTest, DirectoryTransactionMultilineWelcome) {
+  FtpMockControlSocketDirectoryListing ctrl_socket;
+  ctrl_socket.set_multiline_welcome(true);
+  ExecuteTransaction(&ctrl_socket, "ftp://host", OK);
+}
+
 TEST_F(FtpNetworkTransactionTest, DirectoryTransactionShortReads2) {
   FtpMockControlSocketDirectoryListing ctrl_socket;
   ctrl_socket.set_short_read_limit(2);
@@ -305,11 +324,27 @@ TEST_F(FtpNetworkTransactionTest, DirectoryTransactionShortReads5) {
   ExecuteTransaction(&ctrl_socket, "ftp://host", OK);
 }
 
+TEST_F(FtpNetworkTransactionTest, DirectoryTransactionMultilineWelcomeShort) {
+  FtpMockControlSocketDirectoryListing ctrl_socket;
+  // The client will not consume all three 230 lines. That's good, we want to
+  // test that scenario.
+  ctrl_socket.allow_unconsumed_reads(true);
+  ctrl_socket.set_multiline_welcome(true);
+  ctrl_socket.set_short_read_limit(5);
+  ExecuteTransaction(&ctrl_socket, "ftp://host", OK);
+}
+
 TEST_F(FtpNetworkTransactionTest, DownloadTransaction) {
   FtpMockControlSocketFileDownload ctrl_socket;
   ExecuteTransaction(&ctrl_socket, "ftp://host/file", OK);
 }
 
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionMultilineWelcome) {
+  FtpMockControlSocketFileDownload ctrl_socket;
+  ctrl_socket.set_multiline_welcome(true);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", OK);
+}
+
 TEST_F(FtpNetworkTransactionTest, DownloadTransactionShortReads2) {
   FtpMockControlSocketFileDownload ctrl_socket;
   ctrl_socket.set_short_read_limit(2);