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..5a187cef06249dd9f04c65dd3362a35d47366505 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_(ERR_IO_PENDING),
+      transport_write_error_(ERR_IO_PENDING),
       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_ = ERR_IO_PENDING;
+  transport_write_error_ = ERR_IO_PENDING;
 
   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;
@@ -667,6 +667,10 @@ int SSLClientSocketOpenSSL::Init() {
   DCHECK(ssl_bio);
   DCHECK(transport_bio_);
 
+  // Install a callback on OpenSSL's end to plumb transport errors through.
+  BIO_set_callback(ssl_bio, &SSLClientSocketOpenSSL::BIOCallbackThunk);
+  BIO_set_callback_arg(ssl_bio, reinterpret_cast<char*>(this));
+
   SSL_set_bio(ssl_, ssl_bio, ssl_bio);
 
   // OpenSSL defaults some options to on, others to off. To avoid ambiguity,
@@ -785,8 +789,10 @@ 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_ == ERR_IO_PENDING &&
+      BufferRecv() != ERR_IO_PENDING) {
     network_moved = true;
+  }
   return network_moved;
 }
 
@@ -831,7 +837,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 +980,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 +1024,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 +1035,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 +1092,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 +1124,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) {
@@ -1209,20 +1209,9 @@ void SSLClientSocketOpenSSL::BufferRecvComplete(int result) {
 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());
@@ -1237,17 +1226,9 @@ int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   DCHECK(ERR_IO_PENDING != result);
   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 +1240,36 @@ int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   return result;
 }
 
+int SSLClientSocketOpenSSL::HandleOpenSSLError(
+    int ssl_error,
+    const crypto::OpenSSLErrStackTracer& tracer) {
+  // Route transport failures around OpenSSL. Use SSL_ERROR_WANT_READ and
+  // SSL_ERROR_WANT_WRITE to determine whether to use the read or write failure
+  // on the transport. This is done in lieu of routing net errors though
+  // OpenSSL's error system.
+  if (ssl_error == SSL_ERROR_WANT_READ) {
+    // If OpenSSL wanted read data, 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_ != ERR_IO_PENDING)
+      return transport_read_error_;
+    if (transport_write_error_ != ERR_IO_PENDING)
+      return transport_write_error_;
+  } else if (ssl_error == SSL_ERROR_WANT_WRITE) {
+    // Because of the write buffer, this reports a failure from the previous
+    // write payload. If the current payload fails to write, the error will be
+    // reported in a future write or read.
+    if (transport_write_error_ != ERR_IO_PENDING)
+      return transport_write_error_;
+  }
+
+  // Otherwise, map the error code.
+  return MapOpenSSLError(ssl_error, tracer);
+}
+
 int SSLClientSocketOpenSSL::ClientCertRequestCallback(SSL* ssl,
                                                       X509** x509,
                                                       EVP_PKEY** pkey) {
@@ -1431,6 +1442,34 @@ int SSLClientSocketOpenSSL::SelectNextProtoCallback(unsigned char** out,
   return SSL_TLSEXT_ERR_OK;
 }
 
+long SSLClientSocketOpenSSL::BIOCallback(BIO *bio,
+                                         int cmd,
+                                         const char *argp, int argi, long argl,
+                                         long retvalue) {
+  if (cmd == BIO_CB_WRITE && transport_write_error_ != ERR_IO_PENDING) {
+    // If there is a transport write failure, act as if the buffer is full. This
+    // is necessary so future writes are stopped even if there is room in the
+    // buffer. Although retrying will not be of any use, mark as a retry so
+    // SSL_get_error will return SSL_ERROR_WANT_WRITE if there is no other error
+    // in the error queue. HandleOpenSSLError will consume that and route the
+    // transport error code out of OpenSSL.

[davidben, 2014/06/06 23:31:51]

As an alternative, we can BIO_shutdown_wr(SSL_get_wbio(ssl_)) as before and then
rely on the fact that OpenSSL returns BIO_R_BROKEN_PIPE when you try to write
into a BIO you've already shutdown_wr'd.

This seems mildly preferable to me in so far as it's more consistent with how
reads are handled and arguably calling BIO_shutdown_wr(bio); BIO_write(bio); is
a misuse of the API, even though it does check for the case and return a
predictable error code.

Reads don't need any special hooking because the empty buffer behavior and error
behavior are compatible.

[agl, 2014/06/09 17:23:34]

You mean when the write error is detected in TransportWriteComplete? I also
think that's slightly preferable because it avoids this callback, although I
wouldn't worry about it if you're on the fence.

[davidben, 2014/06/10 19:16:30]

Revised to keep the BIO_shutdown_wr and condition on BIO_R_RESET_PIPE.

+    BIO_set_retry_write(bio);
+    return -1;
+  }
+  return retvalue;
+}
+
+// static
+long SSLClientSocketOpenSSL::BIOCallbackThunk(
+    BIO *bio,
+    int cmd,
+    const char *argp, int argi, long argl,
+    long retvalue) {
+  SSLClientSocketOpenSSL* socket = reinterpret_cast<SSLClientSocketOpenSSL*>(
+      BIO_get_callback_arg(bio));
+  return socket->BIOCallback(bio, cmd, argp, argi, argl, retvalue);
+}
+
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;