Close the correct end of the BIO pair on transport send failures in openssl.

Shut down the end of the BIO pair which OpenSSL is writing to, not the
transport socket. However, retain the transport_bio_ shutdown to mirror the
behavior of NSS in r206751, to avoid breaking
SSLClientSocketTest.Write_WithSynchronousError and potentially regressing
http://crbug.com/249848.

To avoid the CHECK in TransportReadComplete, handle the collision of write
failures and pending reads by propogating the write failure.

Remove BIO_set_mem_eof_return calls because they don't do anything. These
aren't mem BIOs.

BUG=335557
TEST=SSLClientSocketTest.Read_WithWriteError

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

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

3 files changed, 135 insertions, 6 deletions

diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index f95fd17..89303d5 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -345,6 +345,7 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       transport_recv_eof_(false),
       weak_factory_(this),
       pending_read_error_(kNoPendingReadResult),
+      transport_write_error_(OK),
       completed_handshake_(false),
       client_auth_cert_needed_(false),
       cert_verifier_(context.cert_verifier),
@@ -468,6 +469,9 @@ void SSLClientSocketOpenSSL::Disconnect() {
   user_write_buf_        = NULL;
   user_write_buf_len_    = 0;
 
+  pending_read_error_ = kNoPendingReadResult;
+  transport_write_error_ = OK;
+
   server_cert_verify_result_.Reset();
   completed_handshake_ = false;
 
@@ -1206,7 +1210,7 @@ int SSLClientSocketOpenSSL::BufferRecv(void) {
   if (rv == ERR_IO_PENDING) {
     transport_recv_busy_ = true;
   } else {
-    TransportReadComplete(rv);
+    rv = TransportReadComplete(rv);
   }
   return rv;
 }
@@ -1218,7 +1222,7 @@ void SSLClientSocketOpenSSL::BufferSendComplete(int result) {
 }
 
 void SSLClientSocketOpenSSL::BufferRecvComplete(int result) {
-  TransportReadComplete(result);
+  result = TransportReadComplete(result);
   OnRecvComplete(result);
 }
 
@@ -1226,9 +1230,19 @@ void SSLClientSocketOpenSSL::TransportWriteComplete(int result) {
   DCHECK(ERR_IO_PENDING != result);
   if (result < 0) {
     // Got a socket write error; close the BIO to indicate this upward.
+    //
+    // TODO(davidben): The value of |result| gets lost. Feed the error back into
+    // the BIO so it gets (re-)detected in OnSendComplete. Perhaps with
+    // BIO_set_callback.
     DVLOG(1) << "TransportWriteComplete error " << result;
+    (void)BIO_shutdown_wr(SSL_get_wbio(ssl_));
+
+    // Match the fix for http://crbug.com/249848 in NSS by erroring future reads
+    // from the socket after a write error.
+    //
+    // TODO(davidben): Avoid having read and write ends interact this way.
+    transport_write_error_ = result;
     (void)BIO_shutdown_wr(transport_bio_);
-    BIO_set_mem_eof_return(transport_bio_, 0);
     send_buffer_ = NULL;
   } else {
     DCHECK(send_buffer_.get());
@@ -1239,7 +1253,7 @@ void SSLClientSocketOpenSSL::TransportWriteComplete(int result) {
   }
 }
 
-void SSLClientSocketOpenSSL::TransportReadComplete(int result) {
+int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   DCHECK(ERR_IO_PENDING != result);
   if (result <= 0) {
     DVLOG(1) << "TransportReadComplete result " << result;
@@ -1248,8 +1262,12 @@ void SSLClientSocketOpenSSL::TransportReadComplete(int result) {
     // relay up to the SSL socket client (i.e. via DoReadCallback).
     if (result == 0)
       transport_recv_eof_ = true;
-    BIO_set_mem_eof_return(transport_bio_, 0);
     (void)BIO_shutdown_wr(transport_bio_);
+  } else if (transport_write_error_ < 0) {
+    // Mirror transport write errors as read failures; transport_bio_ has been
+    // shut down by TransportWriteComplete, so the BIO_write will fail, failing
+    // the CHECK. http://crbug.com/335557.
+    result = transport_write_error_;
   } else {
     DCHECK(recv_buffer_.get());
     int ret = BIO_write(transport_bio_, recv_buffer_->data(), result);
@@ -1261,6 +1279,7 @@ void SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   }
   recv_buffer_ = NULL;
   transport_recv_busy_ = false;
+  return result;
 }
 
 int SSLClientSocketOpenSSL::ClientCertRequestCallback(SSL* ssl,
diff --git a/net/socket/ssl_client_socket_openssl.h b/net/socket/ssl_client_socket_openssl.h
index e391fda..5f4800a 100644
--- a/net/socket/ssl_client_socket_openssl.h
+++ b/net/socket/ssl_client_socket_openssl.h
@@ -121,7 +121,7 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   void BufferSendComplete(int result);
   void BufferRecvComplete(int result);
   void TransportWriteComplete(int result);
-  void TransportReadComplete(int result);
+  int TransportReadComplete(int result);
 
   // Callback from the SSL layer that indicates the remote server is requesting
   // a certificate for this client.
@@ -164,6 +164,10 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   // indicates an error.
   int pending_read_error_;
 
+  // Used by TransportWriteComplete() and TransportReadComplete() to signify an
+  // error writing to the transport socket. A value of OK indicates no error.
+  int transport_write_error_;
+
   // Set when handshake finishes.
   scoped_refptr<X509Certificate> server_cert_;
   CertVerifyResult server_cert_verify_result_;
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index 809ae5e..af05a39 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -1170,6 +1170,112 @@ TEST_F(SSLClientSocketTest, Read_DeleteWhilePendingFullDuplex) {
   EXPECT_FALSE(callback.have_result());
 }
 
+// Tests that the SSLClientSocket does not crash if data is received on the
+// transport socket after a failing write. This can occur if we have a Write
+// error in a SPDY socket.
+// Regression test for http://crbug.com/335557
+TEST_F(SSLClientSocketTest, Read_WithWriteError) {
+  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()));
+  // Note: |error_socket|'s ownership is handed to |transport|, but a pointer
+  // is retained in order to configure additional errors.
+  scoped_ptr<SynchronousErrorStreamSocket> error_socket(
+      new SynchronousErrorStreamSocket(real_transport.Pass()));
+  SynchronousErrorStreamSocket* raw_error_socket = error_socket.get();
+  scoped_ptr<FakeBlockingStreamSocket> transport(
+      new FakeBlockingStreamSocket(error_socket.PassAs<StreamSocket>()));
+  FakeBlockingStreamSocket* raw_transport = transport.get();
+
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  EXPECT_EQ(OK, rv);
+
+  // Disable TLS False Start to avoid handshake non-determinism.
+  SSLConfig ssl_config;
+  ssl_config.false_start_enabled = false;
+
+  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());
+
+  // Send a request so there is something to read from the socket.
+  const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
+  static const int kRequestTextSize =
+      static_cast<int>(arraysize(request_text) - 1);
+  scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kRequestTextSize));
+  memcpy(request_buffer->data(), request_text, kRequestTextSize);
+
+  rv = callback.GetResult(
+      sock->Write(request_buffer.get(), kRequestTextSize, callback.callback()));
+  EXPECT_EQ(kRequestTextSize, rv);
+
+  // Start a hanging read.
+  TestCompletionCallback read_callback;
+  raw_transport->SetNextReadShouldBlock();
+  scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
+  rv = sock->Read(buf.get(), 4096, read_callback.callback());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  // Perform another write, but have it fail. Write a request larger than the
+  // internal socket buffers so that the request hits the underlying transport
+  // socket and detects the error.
+  std::string long_request_text =
+      "GET / HTTP/1.1\r\nUser-Agent: long browser name ";
+  long_request_text.append(20 * 1024, '*');
+  long_request_text.append("\r\n\r\n");
+  scoped_refptr<DrainableIOBuffer> long_request_buffer(new DrainableIOBuffer(
+      new StringIOBuffer(long_request_text), long_request_text.size()));
+
+  raw_error_socket->SetNextWriteError(ERR_CONNECTION_RESET);
+
+  // Write as much data as possible until hitting an error. This is necessary
+  // for NSS. PR_Write will only consume as much data as it can encode into
+  // application data records before the internal memio buffer is full, which
+  // should only fill if writing a large amount of data and the underlying
+  // transport is blocked. Once this happens, NSS will return (total size of all
+  // application data records it wrote) - 1, with the caller expected to resume
+  // with the remaining unsent data.
+  do {
+    rv = callback.GetResult(sock->Write(long_request_buffer.get(),
+                                        long_request_buffer->BytesRemaining(),
+                                        callback.callback()));
+    if (rv > 0) {
+      long_request_buffer->DidConsume(rv);
+      // Abort if the entire buffer is ever consumed.
+      ASSERT_LT(0, long_request_buffer->BytesRemaining());
+    }
+  } while (rv > 0);
+
+#if !defined(USE_OPENSSL)
+  // NSS records the error exactly.
+  EXPECT_EQ(ERR_CONNECTION_RESET, rv);
+#else
+  // OpenSSL treats the reset as a generic protocol error.
+  EXPECT_EQ(ERR_SSL_PROTOCOL_ERROR, rv);
+#endif
+
+  // Release the read. Some bytes should go through.
+  raw_transport->UnblockRead();
+  rv = read_callback.WaitForResult();
+
+  // Per the fix for http://crbug.com/249848, write failures currently break
+  // reads. Change this assertion if they're changed to not collide.
+  EXPECT_EQ(ERR_CONNECTION_RESET, rv);
+}
+
 TEST_F(SSLClientSocketTest, Read_SmallChunks) {
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 SpawnedTestServer::kLocalhost,