Merge 137485 - Prevent the infinite loop inside SSLClientSocketNSS::OnSendComplete.

Two fixes are added. 1) We stay in the loop only if we will call
DoPayloadRead or DoPayloadWrite in the next iteration. 2) Don't call
BufferRecv again if BufferRecv has reported EOF before.

Each fix alone prevents the infinite loop.  The second fix is less risky.
If necessary, we can go with just the second fix.

R=rsleevi@chromium.org
BUG=127822
TEST=SSLServerSocketTest.WriteAfterPeerClose in net_unittests

Review URL: https://chromiumcodereview.appspot.com/10382186

TBR=wtc@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10407008

git-svn-id: svn://svn.chromium.org/chrome/branches/1132/src@137523 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 115 insertions, 8 deletions

diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index b70365d..08192d1 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -431,6 +431,7 @@ SSLClientSocketNSS::SSLClientSocketNSS(ClientSocketHandle* transport_socket,
                                        const SSLClientSocketContext& context)
     : transport_send_busy_(false),
       transport_recv_busy_(false),
+      transport_recv_eof_(false),
       transport_(transport_socket),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
@@ -634,6 +635,7 @@ void SSLClientSocketNSS::Disconnect() {
   user_write_callback_.Reset();
   transport_send_busy_   = false;
   transport_recv_busy_   = false;
+  transport_recv_eof_    = false;
   user_read_buf_         = NULL;
   user_read_buf_len_     = 0;
   user_write_buf_        = NULL;
@@ -1222,6 +1224,7 @@ void SSLClientSocketNSS::OnSendComplete(int result) {
       network_moved = DoTransportIO();
   } while (rv_read == ERR_IO_PENDING &&
            rv_write == ERR_IO_PENDING &&
+           (user_read_buf_ || user_write_buf_) &&
            network_moved);
 
   if (user_read_buf_ && rv_read != ERR_IO_PENDING)
@@ -1919,7 +1922,7 @@ bool SSLClientSocketNSS::DoTransportIO() {
       if (rv > 0)
         network_moved = true;
     } while (rv > 0);
-    if (BufferRecv() >= 0)
+    if (!transport_recv_eof_ && BufferRecv() >= 0)
       network_moved = true;
   }
   LeaveFunction(network_moved);
@@ -1929,7 +1932,7 @@ bool SSLClientSocketNSS::DoTransportIO() {
 // Return 0 for EOF,
 // > 0 for bytes transferred immediately,
 // < 0 for error (or the non-error ERR_IO_PENDING).
-int SSLClientSocketNSS::BufferSend(void) {
+int SSLClientSocketNSS::BufferSend() {
   if (transport_send_busy_)
     return ERR_IO_PENDING;
 
@@ -1968,7 +1971,7 @@ void SSLClientSocketNSS::BufferSendComplete(int result) {
   LeaveFunction("");
 }
 
-int SSLClientSocketNSS::BufferRecv(void) {
+int SSLClientSocketNSS::BufferRecv() {
   if (transport_recv_busy_) return ERR_IO_PENDING;
 
   char* buf;
@@ -1987,8 +1990,11 @@ int SSLClientSocketNSS::BufferRecv(void) {
     if (rv == ERR_IO_PENDING) {
       transport_recv_busy_ = true;
     } else {
-      if (rv > 0)
+      if (rv > 0) {
         memcpy(buf, recv_buffer_->data(), rv);
+      } else if (rv == 0) {
+        transport_recv_eof_ = true;
+      }
       memio_PutReadResult(nss_bufs_, MapErrorToNSS(rv));
       recv_buffer_ = NULL;
     }
@@ -2003,6 +2009,8 @@ void SSLClientSocketNSS::BufferRecvComplete(int result) {
     char* buf;
     memio_GetReadParams(nss_bufs_, &buf);
     memcpy(buf, recv_buffer_->data(), result);
+  } else if (result == 0) {
+    transport_recv_eof_ = true;
   }
   recv_buffer_ = NULL;
   memio_PutReadResult(nss_bufs_, MapErrorToNSS(result));
diff --git a/net/socket/ssl_client_socket_nss.h b/net/socket/ssl_client_socket_nss.h
index b708504..10c5eab 100644
--- a/net/socket/ssl_client_socket_nss.h
+++ b/net/socket/ssl_client_socket_nss.h
@@ -149,9 +149,9 @@ class SSLClientSocketNSS : public SSLClientSocket {
   void SaveSSLHostInfo();
 
   bool DoTransportIO();
-  int BufferSend(void);
+  int BufferSend();
   void BufferSendComplete(int result);
-  int BufferRecv(void);
+  int BufferRecv();
   void BufferRecvComplete(int result);
 
   // Handles an NSS error generated while handshaking or performing IO.
@@ -210,6 +210,7 @@ class SSLClientSocketNSS : public SSLClientSocket {
 
   bool transport_send_busy_;
   bool transport_recv_busy_;
+  bool transport_recv_eof_;
   scoped_refptr<IOBuffer> recv_buffer_;
 
   scoped_ptr<ClientSocketHandle> transport_;
diff --git a/net/socket/ssl_server_socket_unittest.cc b/net/socket/ssl_server_socket_unittest.cc
index 97841ff..53af569 100644
--- a/net/socket/ssl_server_socket_unittest.cc
+++ b/net/socket/ssl_server_socket_unittest.cc
@@ -54,10 +54,14 @@ class FakeDataChannel {
  public:
   FakeDataChannel()
       : read_buf_len_(0),
-        ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
+        ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
+        closed_(false),
+        write_called_after_close_(false) {
   }
 
   int Read(IOBuffer* buf, int buf_len, const CompletionCallback& callback) {
+    if (closed_)
+      return 0;
     if (data_.empty()) {
       read_callback_ = callback;
       read_buf_ = buf;
@@ -68,6 +72,16 @@ class FakeDataChannel {
   }
 
   int Write(IOBuffer* buf, int buf_len, const CompletionCallback& callback) {
+    if (closed_) {
+      if (write_called_after_close_)
+        return net::ERR_CONNECTION_RESET;
+      write_called_after_close_ = true;
+      write_callback_ = callback;
+      MessageLoop::current()->PostTask(
+          FROM_HERE, base::Bind(&FakeDataChannel::DoWriteCallback,
+                                weak_factory_.GetWeakPtr()));
+      return net::ERR_IO_PENDING;
+    }
     data_.push(new net::DrainableIOBuffer(buf, buf_len));
     MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(&FakeDataChannel::DoReadCallback,
@@ -75,6 +89,14 @@ class FakeDataChannel {
     return buf_len;
   }
 
+  // Closes the FakeDataChannel. After Close() is called, Read() returns 0,
+  // indicating EOF, and Write() fails with ERR_CONNECTION_RESET. Note that
+  // after the FakeDataChannel is closed, the first Write() call completes
+  // asynchronously, which is necessary to reproduce bug 127822.
+  void Close() {
+    closed_ = true;
+  }
+
  private:
   void DoReadCallback() {
     if (read_callback_.is_null() || data_.empty())
@@ -88,6 +110,15 @@ class FakeDataChannel {
     callback.Run(copied);
   }
 
+  void DoWriteCallback() {
+    if (write_callback_.is_null())
+      return;
+
+    CompletionCallback callback = write_callback_;
+    write_callback_.Reset();
+    callback.Run(net::ERR_CONNECTION_RESET);
+  }
+
   int PropogateData(scoped_refptr<net::IOBuffer> read_buf, int read_buf_len) {
     scoped_refptr<net::DrainableIOBuffer> buf = data_.front();
     int copied = std::min(buf->BytesRemaining(), read_buf_len);
@@ -103,10 +134,20 @@ class FakeDataChannel {
   scoped_refptr<net::IOBuffer> read_buf_;
   int read_buf_len_;
 
+  CompletionCallback write_callback_;
+
   std::queue<scoped_refptr<net::DrainableIOBuffer> > data_;
 
   base::WeakPtrFactory<FakeDataChannel> weak_factory_;
 
+  // True if Close() has been called.
+  bool closed_;
+
+  // Controls the completion of Write() after the FakeDataChannel is closed.
+  // After the FakeDataChannel is closed, the first Write() call completes
+  // asynchronously.
+  bool write_called_after_close_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeDataChannel);
 };
 
@@ -147,7 +188,10 @@ class FakeSocket : public StreamSocket {
     return net::OK;
   }
 
-  virtual void Disconnect() OVERRIDE {}
+  virtual void Disconnect() OVERRIDE {
+    incoming_->Close();
+    outgoing_->Close();
+  }
 
   virtual bool IsConnected() const OVERRIDE {
     return true;
@@ -429,6 +473,60 @@ TEST_F(SSLServerSocketTest, DataTransfer) {
   EXPECT_EQ(0, memcmp(write_buf->data(), read_buf->data(), write_buf->size()));
 }
 
+// A regression test for bug 127822 (http://crbug.com/127822).
+// If the server closes the connection after the handshake is finished,
+// the client's Write() call should not cause an infinite loop.
+// NOTE: this is a test for SSLClientSocket rather than SSLServerSocket.
+TEST_F(SSLServerSocketTest, ClientWriteAfterServerClose) {
+  Initialize();
+
+  TestCompletionCallback connect_callback;
+  TestCompletionCallback handshake_callback;
+
+  // Establish connection.
+  int client_ret = client_socket_->Connect(connect_callback.callback());
+  ASSERT_TRUE(client_ret == net::OK || client_ret == net::ERR_IO_PENDING);
+
+  int server_ret = server_socket_->Handshake(handshake_callback.callback());
+  ASSERT_TRUE(server_ret == net::OK || server_ret == net::ERR_IO_PENDING);
+
+  client_ret = connect_callback.GetResult(client_ret);
+  ASSERT_EQ(net::OK, client_ret);
+  server_ret = handshake_callback.GetResult(server_ret);
+  ASSERT_EQ(net::OK, server_ret);
+
+  scoped_refptr<net::StringIOBuffer> write_buf =
+      new net::StringIOBuffer("testing123");
+
+  // The server closes the connection. The server needs to write some
+  // data first so that the client's Read() calls from the transport
+  // socket won't return ERR_IO_PENDING.  This ensures that the client
+  // will call Read() on the transport socket again.
+  TestCompletionCallback write_callback;
+
+  server_ret = server_socket_->Write(write_buf, write_buf->size(),
+                                     write_callback.callback());
+  EXPECT_TRUE(server_ret > 0 || server_ret == net::ERR_IO_PENDING);
+
+  server_ret = write_callback.GetResult(server_ret);
+  EXPECT_GT(server_ret, 0);
+
+  server_socket_->Disconnect();
+
+  // The client writes some data. This should not cause an infinite loop.
+  client_ret = client_socket_->Write(write_buf, write_buf->size(),
+                                     write_callback.callback());
+  EXPECT_TRUE(client_ret > 0 || client_ret == net::ERR_IO_PENDING);
+
+  client_ret = write_callback.GetResult(client_ret);
+  EXPECT_GT(client_ret, 0);
+
+  MessageLoop::current()->PostDelayedTask(
+      FROM_HERE, MessageLoop::QuitClosure(),
+      base::TimeDelta::FromMilliseconds(10));
+  MessageLoop::current()->Run();
+}
+
 // This test executes ExportKeyingMaterial() on the client and server sockets,
 // after connecting them, and verifies that the results match.
 // This test will fail if False Start is enabled (see crbug.com/90208).