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

4 files changed, 85 insertions, 20 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);
diff --git a/net/socket/socket_test_util.cc b/net/socket/socket_test_util.cc
index fc5ddf3..25de9d3 100644
--- a/net/socket/socket_test_util.cc
+++ b/net/socket/socket_test_util.cc
@@ -248,33 +248,33 @@ void StaticMockSocket::Reset() {
 }
 
 DynamicMockSocket::DynamicMockSocket()
-    : read_(false, ERR_UNEXPECTED),
-      has_read_(false),
-      short_read_limit_(0) {
+    : short_read_limit_(0),
+      allow_unconsumed_reads_(false) {
 }
 
 MockRead DynamicMockSocket::GetNextRead() {
-  if (!has_read_)
+  if (reads_.empty())
     return MockRead(true, ERR_UNEXPECTED);
-  MockRead result = read_;
+  MockRead result = reads_.front();
   if (short_read_limit_ == 0 || result.data_len <= short_read_limit_) {
-    has_read_ = false;
+    reads_.pop_front();
   } else {
     result.data_len = short_read_limit_;
-    read_.data += result.data_len;
-    read_.data_len -= result.data_len;
+    reads_.front().data += result.data_len;
+    reads_.front().data_len -= result.data_len;
   }
   return result;
 }
 
 void DynamicMockSocket::Reset() {
-  has_read_ = false;
+  reads_.clear();
 }
 
 void DynamicMockSocket::SimulateRead(const char* data) {
-  EXPECT_FALSE(has_read_) << "Unconsumed read: " << read_.data;
-  read_ = MockRead(data);
-  has_read_ = true;
+  if (!allow_unconsumed_reads_) {
+    EXPECT_TRUE(reads_.empty()) << "Unconsumed read: " << reads_.front().data;
+  }
+  reads_.push_back(MockRead(data));
 }
 
 void MockClientSocketFactory::AddMockSocket(MockSocket* socket) {
diff --git a/net/socket/socket_test_util.h b/net/socket/socket_test_util.h
index fc288ad..93f9625 100644
--- a/net/socket/socket_test_util.h
+++ b/net/socket/socket_test_util.h
@@ -5,6 +5,7 @@
 #ifndef NET_SOCKET_SOCKET_TEST_UTIL_H_
 #define NET_SOCKET_SOCKET_TEST_UTIL_H_
 
+#include <deque>
 #include <string>
 #include <vector>
 
@@ -124,18 +125,23 @@ class DynamicMockSocket : public MockSocket {
   int short_read_limit() const { return short_read_limit_; }
   void set_short_read_limit(int limit) { short_read_limit_ = limit; }
 
+  void allow_unconsumed_reads(bool allow) { allow_unconsumed_reads_ = allow; }
+
  protected:
   // The next time there is a read from this socket, it will return |data|.
   // Before calling SimulateRead next time, the previous data must be consumed.
   void SimulateRead(const char* data);
 
  private:
-  MockRead read_;
-  bool has_read_;
+  std::deque<MockRead> reads_;
 
   // Max number of bytes we will read at a time. 0 means no limit.
   int short_read_limit_;
 
+  // If true, we'll not require the client to consume all data before we
+  // mock the next read.
+  bool allow_unconsumed_reads_;
+
   DISALLOW_COPY_AND_ASSIGN(DynamicMockSocket);
 };