From be6ce7ecd6d31721576d39e65d74fdbda65d48f1 Mon Sep 17 00:00:00 2001
From: davidben <davidben@chromium.org>
Date: Mon, 20 Oct 2014 12:15:56 -0700
Subject: [PATCH] Close SSLClientSocketOpenSSL cleanly if the transport was
 closed.

Servers do not reliably send close_notify. This regressed in
https://codereview.chromium.org/367963007 which had a side effect of making us
sensitive to this.

BUG=422246

Review URL: https://codereview.chromium.org/655813003

Cr-Commit-Position: refs/heads/master@{#300308}
---
 net/socket/ssl_client_socket_openssl.cc  |  9 ++++
 net/socket/ssl_client_socket_unittest.cc | 76 +++++++++++++++++++++++++++++++-
 2 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index 04be9881e170..953f3363806c 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -1363,6 +1363,15 @@ int SSLClientSocketOpenSSL::DoPayloadRead() {
     } else if (*next_result < 0) {
       int err = SSL_get_error(ssl_, *next_result);
       *next_result = MapOpenSSLError(err, err_tracer);
+
+      // Many servers do not reliably send a close_notify alert when shutting
+      // down a connection, and instead terminate the TCP connection. This is
+      // reported as ERR_CONNECTION_CLOSED. Because of this, map the unclean
+      // shutdown to a graceful EOF, instead of treating it as an error as it
+      // should be.
+      if (*next_result == ERR_CONNECTION_CLOSED)
+        *next_result = 0;
+
       if (rv > 0 && *next_result == ERR_IO_PENDING) {
           // If at least some data was read from SSL_read(), do not treat
           // insufficient data as an error to return in the next call to
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index 7e7bf2cd115a..58e43c75f47b 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -276,7 +276,7 @@ class SynchronousErrorStreamSocket : public WrappedStreamSocket {
   // If there is already a pending asynchronous read, the configured error
   // will not be returned until that asynchronous read has completed and Read()
   // is called again.
-  void SetNextReadError(Error error) {
+  void SetNextReadError(int error) {
     DCHECK_GE(0, error);
     have_read_error_ = true;
     pending_read_error_ = error;
@@ -286,7 +286,7 @@ class SynchronousErrorStreamSocket : public WrappedStreamSocket {
   // If there is already a pending asynchronous write, the configured error
   // will not be returned until that asynchronous write has completed and
   // Write() is called again.
-  void SetNextWriteError(Error error) {
+  void SetNextWriteError(int error) {
     DCHECK_GE(0, error);
     have_write_error_ = true;
     pending_write_error_ = error;
@@ -1813,6 +1813,78 @@ TEST_F(SSLClientSocketTest, Read_WithWriteError) {
 #endif
 }
 
+// Tests that SSLClientSocket fails the handshake if the underlying
+// transport is cleanly closed.
+TEST_F(SSLClientSocketTest, Connect_WithZeroReturn) {
+  SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
+                                SpawnedTestServer::kLocalhost,
+                                base::FilePath());
+  ASSERT_TRUE(test_server.Start());
+
+  AddressList addr;
+  ASSERT_TRUE(test_server.GetAddressList(&addr));
+
+  TestCompletionCallback callback;
+  scoped_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr, NULL, NetLog::Source()));
+  scoped_ptr<SynchronousErrorStreamSocket> transport(
+      new SynchronousErrorStreamSocket(real_transport.Pass()));
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  EXPECT_EQ(OK, rv);
+
+  SynchronousErrorStreamSocket* raw_transport = transport.get();
+  scoped_ptr<SSLClientSocket> sock(
+      CreateSSLClientSocket(transport.PassAs<StreamSocket>(),
+                            test_server.host_port_pair(),
+                            kDefaultSSLConfig));
+
+  raw_transport->SetNextReadError(0);
+
+  rv = callback.GetResult(sock->Connect(callback.callback()));
+  EXPECT_EQ(ERR_CONNECTION_CLOSED, rv);
+  EXPECT_FALSE(sock->IsConnected());
+}
+
+// Tests that SSLClientSocket cleanly returns a Read of size 0 if the
+// underlying socket is cleanly closed.
+// This is a regression test for https://crbug.com/422246
+TEST_F(SSLClientSocketTest, Read_WithZeroReturn) {
+  SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
+                                SpawnedTestServer::kLocalhost,
+                                base::FilePath());
+  ASSERT_TRUE(test_server.Start());
+
+  AddressList addr;
+  ASSERT_TRUE(test_server.GetAddressList(&addr));
+
+  TestCompletionCallback callback;
+  scoped_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr, NULL, NetLog::Source()));
+  scoped_ptr<SynchronousErrorStreamSocket> transport(
+      new SynchronousErrorStreamSocket(real_transport.Pass()));
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  EXPECT_EQ(OK, rv);
+
+  // Disable TLS False Start to ensure the handshake has completed.
+  SSLConfig ssl_config;
+  ssl_config.false_start_enabled = false;
+
+  SynchronousErrorStreamSocket* raw_transport = transport.get();
+  scoped_ptr<SSLClientSocket> sock(
+      CreateSSLClientSocket(transport.PassAs<StreamSocket>(),
+                            test_server.host_port_pair(),
+                            ssl_config));
+
+  rv = callback.GetResult(sock->Connect(callback.callback()));
+  EXPECT_EQ(OK, rv);
+  EXPECT_TRUE(sock->IsConnected());
+
+  raw_transport->SetNextReadError(0);
+  scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
+  rv = callback.GetResult(sock->Read(buf.get(), 4096, callback.callback()));
+  EXPECT_EQ(0, rv);
+}
+
 TEST_F(SSLClientSocketTest, Read_SmallChunks) {
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 SpawnedTestServer::kLocalhost,
-- 
2.11.4.GIT