Preserve transport errors for OpenSSL sockets.

net/socket/ssl_client_socket_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/ssl_client_socket.h"
 
 #include "base/callback_helpers.h"
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
 #include "base/time/time.h"
@@ skipping 1228 matching lines @@
 
     if (rv == ERR_IO_PENDING)
       rv = callback.WaitForResult();
 
     EXPECT_GE(rv, 0);
     if (rv <= 0)
       break;
   }
 }
 
+// Tests that SSLClientSocket properly handles when the underlying transport
+// synchronously fails a transport read in during the handshake. The error code
+// should be preserved so SSLv3 fallback logic can condition on it.
+TEST_F(SSLClientSocketTest, Connect_WithSynchronousError) {
+  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 avoid handshake non-determinism.
+  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));
+
+  raw_transport->SetNextWriteError(ERR_CONNECTION_RESET);
+
+  rv = callback.GetResult(sock->Connect(callback.callback()));
+  EXPECT_EQ(ERR_CONNECTION_RESET, rv);
+  EXPECT_FALSE(sock->IsConnected());
+}
+
 // Tests that the SSLClientSocket properly handles when the underlying transport
 // synchronously returns an error code - such as if an intermediary terminates
 // the socket connection uncleanly.
 // This is a regression test for http://crbug.com/238536
 TEST_F(SSLClientSocketTest, Read_WithSynchronousError) {
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 SpawnedTestServer::kLocalhost,
                                 base::FilePath());
   ASSERT_TRUE(test_server.Start());
 
@@ skipping 34 matching lines @@
 
   // Simulate an unclean/forcible shutdown.
   raw_transport->SetNextReadError(ERR_CONNECTION_RESET);
 
   scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
 
   // Note: This test will hang if this bug has regressed. Simply checking that
   // rv != ERR_IO_PENDING is insufficient, as ERR_IO_PENDING is a legitimate
   // result when using a dedicated task runner for NSS.
   rv = callback.GetResult(sock->Read(buf.get(), 4096, callback.callback()));
-
-#if !defined(USE_OPENSSL)
-  // SSLClientSocketNSS records the error exactly
   EXPECT_EQ(ERR_CONNECTION_RESET, rv);
-#else
-  // SSLClientSocketOpenSSL treats any errors as a simple EOF.
-  EXPECT_EQ(0, rv);
-#endif
 }
 
 // Tests that the SSLClientSocket properly handles when the underlying transport
 // asynchronously returns an error code while writing data - such as if an
 // intermediary terminates the socket connection uncleanly.
 // This is a regression test for http://crbug.com/249848
 TEST_F(SSLClientSocketTest, Write_WithSynchronousError) {
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 SpawnedTestServer::kLocalhost,
                                 base::FilePath());
@@ skipping 53 matching lines @@
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
   // Now unblock the outgoing request, having it fail with the connection
   // being reset.
   raw_transport->UnblockWrite();
 
   // Note: This will cause an inifite loop if this bug has regressed. Simply
   // checking that rv != ERR_IO_PENDING is insufficient, as ERR_IO_PENDING
   // is a legitimate result when using a dedicated task runner for NSS.
   rv = callback.GetResult(rv);
-
-#if !defined(USE_OPENSSL)
-  // SSLClientSocketNSS records the error exactly
   EXPECT_EQ(ERR_CONNECTION_RESET, rv);
-#else
-  // SSLClientSocketOpenSSL treats any errors as a simple EOF.
-  EXPECT_EQ(0, rv);
-#endif
 }
 
 // If there is a Write failure at the transport with no follow-up Read, although
 // the write error will not be returned to the client until a future Read or
 // Write operation, SSLClientSocket should not spin attempting to re-write on
 // the socket. This is a regression test for part of https://crbug.com/381160.
 TEST_F(SSLClientSocketTest, Write_WithSynchronousErrorNoRead) {
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 SpawnedTestServer::kLocalhost,
                                 base::FilePath());
@@ skipping 222 matching lines @@
                        callback.callback());
   ASSERT_EQ(ERR_IO_PENDING, rv);
   ASSERT_FALSE(callback.have_result());
 
   // Now unblock Write(), which will invoke OnSendComplete and (eventually)
   // call the Read() callback, deleting the socket and thus aborting calling
   // the Write() callback.
   raw_transport->UnblockWrite();
 
   rv = read_callback.WaitForResult();
-
-#if !defined(USE_OPENSSL)
-  // NSS records the error exactly.
   EXPECT_EQ(ERR_CONNECTION_RESET, rv);
-#else
-  // OpenSSL treats any errors as a simple EOF.
-  EXPECT_EQ(0, rv);
-#endif
 
   // The Write callback should not have been called.
   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) {
@@ skipping 74 matching lines @@
     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.
+  // Release the read.
   raw_transport->UnblockReadResult();
   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.
+#if defined(USE_OPENSSL)
+  // Should still read bytes despite the write error.
+  EXPECT_LT(0, rv);
+#else
+  // NSS attempts to flush the write buffer in PR_Read on an SSL socket before
+  // pumping the read state machine, unless configured with SSL_ENABLE_FDX, so
+  // the write error stops future reads.

[Ryan Sleevi, 2014/07/07 22:36:34]

wtc: HERE

[wtc, 2014/07/09 15:55:27]

I think we should fix this. This shouldn't be too hard.

David, have you looked into this? I think we can have ssl_SecureRecv go on to
DoRecv even if the ssl_SendSavedWriteData call fails. But we may need to save
the error code of the ssl_SendSavedWriteData call in sslSocket and report that
error at the "right time", rather than calling ssl_SendSavedWriteData again.

[davidben, 2014/07/09 16:00:08]

I haven't looked at it much, apart from finding the bit in the code that does
this. What if we set SSL_ENABLE_FDX? I dunno what else it does or if that logic
is actually desirable to begin with.

   EXPECT_EQ(ERR_CONNECTION_RESET, rv);
+#endif
 }
 
 TEST_F(SSLClientSocketTest, Read_SmallChunks) {
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 SpawnedTestServer::kLocalhost,
                                 base::FilePath());
   ASSERT_TRUE(test_server.Start());
 
   AddressList addr;
   ASSERT_TRUE(test_server.GetAddressList(&addr));
@@ skipping 1030 matching lines @@
   ssl_config.channel_id_enabled = true;
 
   int rv;
   ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));
 
   EXPECT_EQ(ERR_UNEXPECTED, rv);
   EXPECT_FALSE(sock_->IsConnected());
 }
 
 }  // namespace net