Preserve transport errors for OpenSSL sockets.

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index 4ff8d438e965b1fdf0b5b7f8b4ecdb75e1d14d66..fe88dfacb178730b601309fb836795e8465f1477 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -339,10 +339,10 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
     const SSLClientSocketContext& context)
     : transport_send_busy_(false),
       transport_recv_busy_(false),
-      transport_recv_eof_(false),
       weak_factory_(this),
       pending_read_error_(kNoPendingReadResult),
-      transport_write_error_(OK),
+      transport_read_error_(0),
+      transport_write_error_(0),
       server_cert_chain_(new PeerCertificateChain(NULL)),
       completed_handshake_(false),
       was_ever_used_(false),
@@ -401,7 +401,7 @@ int SSLClientSocketOpenSSL::ExportKeyingMaterial(
     LOG(ERROR) << "Failed to export keying material;"
                << " returned " << rv
                << ", SSL error code " << ssl_error;
-    return MapOpenSSLError(ssl_error, err_tracer);
+    return HandleOpenSSLError(ssl_error, err_tracer);
   }
   return OK;
 }
@@ -456,7 +456,6 @@ void SSLClientSocketOpenSSL::Disconnect() {
   transport_send_busy_ = false;
   send_buffer_ = NULL;
   transport_recv_busy_ = false;
-  transport_recv_eof_ = false;
   recv_buffer_ = NULL;
 
   user_connect_callback_.Reset();
@@ -468,7 +467,8 @@ void SSLClientSocketOpenSSL::Disconnect() {
   user_write_buf_len_    = 0;
 
   pending_read_error_ = kNoPendingReadResult;
-  transport_write_error_ = OK;
+  transport_read_error_ = 0;
+  transport_write_error_ = 0;
 
   server_cert_verify_result_.Reset();
   completed_handshake_ = false;
@@ -601,7 +601,7 @@ int SSLClientSocketOpenSSL::Read(IOBuffer* buf,
   user_read_buf_ = buf;
   user_read_buf_len_ = buf_len;
 
-  int rv = DoReadLoop(OK);
+  int rv = DoReadLoop();
 
   if (rv == ERR_IO_PENDING) {
     user_read_callback_ = callback;
@@ -621,7 +621,7 @@ int SSLClientSocketOpenSSL::Write(IOBuffer* buf,
   user_write_buf_ = buf;
   user_write_buf_len_ = buf_len;
 
-  int rv = DoWriteLoop(OK);
+  int rv = DoWriteLoop();
 
   if (rv == ERR_IO_PENDING) {
     user_write_callback_ = callback;
@@ -785,7 +785,7 @@ bool SSLClientSocketOpenSSL::DoTransportIO() {
     if (rv != ERR_IO_PENDING && rv != 0)
       network_moved = true;
   } while (rv > 0);
-  if (!transport_recv_eof_ && BufferRecv() != ERR_IO_PENDING)
+  if (transport_read_error_ == 0 && BufferRecv() != ERR_IO_PENDING)
     network_moved = true;
   return network_moved;
 }
@@ -831,7 +831,7 @@ int SSLClientSocketOpenSSL::DoHandshake() {
       // Retrieve the error from the call to |server_bound_cert_service_|.
       net_error = channel_id_request_return_value_;
     } else {
-      net_error = MapOpenSSLError(ssl_error, err_tracer);
+      net_error = HandleOpenSSLError(ssl_error, err_tracer);
     }
 
     // If not done, stay in this state
@@ -974,7 +974,7 @@ void SSLClientSocketOpenSSL::OnRecvComplete(int result) {
   if (!user_read_buf_.get())
     return;
 
-  int rv = DoReadLoop(result);
+  int rv = DoReadLoop();
   if (rv != ERR_IO_PENDING)
     DoReadCallback(rv);
 }
@@ -1018,10 +1018,7 @@ int SSLClientSocketOpenSSL::DoHandshakeLoop(int last_io_result) {
   return rv;
 }
 
-int SSLClientSocketOpenSSL::DoReadLoop(int result) {
-  if (result < 0)
-    return result;
-
+int SSLClientSocketOpenSSL::DoReadLoop() {
   bool network_moved;
   int rv;
   do {
@@ -1032,10 +1029,7 @@ int SSLClientSocketOpenSSL::DoReadLoop(int result) {
   return rv;
 }
 
-int SSLClientSocketOpenSSL::DoWriteLoop(int result) {
-  if (result < 0)
-    return result;
-
+int SSLClientSocketOpenSSL::DoWriteLoop() {
   bool network_moved;
   int rv;
   do {
@@ -1092,15 +1086,15 @@ int SSLClientSocketOpenSSL::DoPayloadRead() {
       *next_result = ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
     } else if (*next_result < 0) {
       int err = SSL_get_error(ssl_, *next_result);
-      *next_result = MapOpenSSLError(err, err_tracer);
+      *next_result = HandleOpenSSLError(err, err_tracer);
       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
-          // DoPayloadRead() - instead, let the call fall through to check
-          // SSL_read() again. This is because DoTransportIO() may complete
-          // in between the next call to DoPayloadRead(), and thus it is
-          // important to check SSL_read() on subsequent invocations to see
-          // if a complete record may now be read.
+        // 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
+        // DoPayloadRead() - instead, let the call fall through to check
+        // SSL_read() again. This is because DoTransportIO() may complete
+        // in between the next call to DoPayloadRead(), and thus it is
+        // important to check SSL_read() on subsequent invocations to see
+        // if a complete record may now be read.
         *next_result = kNoPendingReadResult;
       }
     }
@@ -1124,7 +1118,7 @@ int SSLClientSocketOpenSSL::DoPayloadWrite() {
   }
 
   int err = SSL_get_error(ssl_, rv);
-  return MapOpenSSLError(err, err_tracer);
+  return HandleOpenSSLError(err, err_tracer);
 }
 
 int SSLClientSocketOpenSSL::BufferSend(void) {
@@ -1210,19 +1204,12 @@ 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.
+    // Record the error. Save it to be reported in a future read or write on
+    // transport_bio_'s peer.
     transport_write_error_ = result;
-    (void)BIO_shutdown_wr(transport_bio_);
     send_buffer_ = NULL;
   } else {
     DCHECK(send_buffer_.get());
@@ -1235,19 +1222,15 @@ void SSLClientSocketOpenSSL::TransportWriteComplete(int result) {
 
 int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   DCHECK(ERR_IO_PENDING != result);
-  if (result <= 0) {
+  // If an EOF, canonicalize to ERR_CONNECTION_CLOSED here so HandleOpenSSLError
+  // does not report success.
+  if (result == 0)
+    result = ERR_CONNECTION_CLOSED;
+  if (result < 0) {
     DVLOG(1) << "TransportReadComplete result " << result;
-    // Received 0 (end of file) or an error. Either way, bubble it up to the
-    // SSL layer via the BIO. TODO(joth): consider stashing the error code, to
-    // relay up to the SSL socket client (i.e. via DoReadCallback).
-    if (result == 0)
-      transport_recv_eof_ = true;
-    (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_;
+    // Received 0 (end of file) or an error. Save it to be reported in a future
+    // read on transport_bio_'s peer.
+    transport_read_error_ = result;
   } else {
     DCHECK(recv_buffer_.get());
     int ret = BIO_write(transport_bio_, recv_buffer_->data(), result);
@@ -1259,6 +1242,56 @@ int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   return result;
 }
 
+int SSLClientSocketOpenSSL::HandleOpenSSLError(
+    int ssl_error,
+    const crypto::OpenSSLErrStackTracer& tracer) {
+  // Normal OpenSSL error-handling requires handling |ssl_error| being
+  // SSL_ERROR_SYSCALL by, after checking the error queue, etc., checking
+  // |errno|, or the equivalent, for the underlying transport error. This would

[Ryan Sleevi, 2014/06/19 00:27:06]

Having trouble parsing one bit of this

// Normal OpenSSL error-handling works by having
// |ssl_error| being set to SSL_ERROR_SYSCALL. The application
// is then expected to first check the error queue, and, if
// empty, checking |errno| or equivalent for the underlying
// transport error.
//
// To do this with the BIO would require hooking into the
// BIO_read/BIO_write calls to OpenSSL's half of |transport_bio_|, in order to
determine whether to
// extract |transport_read_error_| or |transport_write_error_|

(side note: Not sure I fully grok the "require hooking into BIO_read/BIO_write
calls to OpenSSL". Are you saying OpenSSL would have to be modified? You'd have
to create a custom BIO? Something else?)

+  // require hooking into BIO_read and BIO_write calls to OpenSSL's end of

[Ryan Sleevi, 2014/06/19 00:27:06]

Ok, let's use "SSL*" rather than "OpenSSL" for portions where you're talking
about OpenSSL having the SSL* call BIO_read/BIO_write.

I think that means line 1251, 1260, and 1272/1273/1277/1280?

+  // |transport_bio_| to determine which of |transport_read_error_| or
+  // |transport_write_error_| to extract.
+  //
+  // Instead, determine if the OpenSSL operation failed because a BIO_read or
+  // BIO_write failed. Report the appropriate saved transport error code in that
+  // case.
+  if (ssl_error == SSL_ERROR_WANT_READ) {
+    // On transport read failure, the |transport_bio_| is intentionally not
+    // closed, so OpenSSL will continue to report SSL_ERROR_WANT_READ once
+    // exhausting the read buffer. Report any pending errors that were
+    // observed. Note that both |transport_read_error_| and
+    // |transport_write_error_| are checked, since the application may have
+    // encountered a socket error while writing that would otherwise not be
+    // reported until the application attempted to write again - which it may
+    // never do. This matches the fix for https://crbug.com/249848 in NSS.
+    if (transport_read_error_ != 0)
+      return transport_read_error_;
+    if (transport_write_error_ != 0)
+      return transport_write_error_;
+  } else if (ssl_error == SSL_ERROR_SSL) {
+    // On transport write failure, OpenSSL's end of |transport_bio_| is shutdown
+    // for future writes. If OpenSSL then attempts again to BIO_write, it will
+    // fail with BIO_R_BROKEN_PIPE. Map this to the saved transport write error.
+    //
+    // Note that, because of the write buffer, this reports a failure from
+    // OpenSSL's previous BIO_write call. BIO_write on OpenSSL's end of
+    // |transport_bio_| returns successfully as soon as data is committed to the
+    // write buffer. If it then fails to be written to the network, the error is
+    // not reported until OpenSSL's next BIO_write or BIO_read call. From there,
+    // it bubbles up to SSLClientSocketOpenSSL's caller through this function.
+    unsigned long error = ERR_peek_error();
+    if (error &&
+        ERR_GET_LIB(error) == ERR_LIB_BIO &&
+        ERR_GET_REASON(error) == BIO_R_BROKEN_PIPE) {
+      DCHECK_NE(0, transport_write_error_);
+      return transport_write_error_;
+    }
+  }
+
+  // Otherwise, map the error code.
+  return MapOpenSSLError(ssl_error, tracer);
+}
+
 int SSLClientSocketOpenSSL::ClientCertRequestCallback(SSL* ssl,
                                                       X509** x509,
                                                       EVP_PKEY** pkey) {