Preserve transport errors for OpenSSL sockets.

net/socket/ssl_client_socket_openssl.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.
 
 // OpenSSL binding for SSLClientSocket. The class layout and general principle
 // of operation is derived from SSLClientSocketNSS.
 
 #include "net/socket/ssl_client_socket_openssl.h"
 
 #include <openssl/err.h>
@@ skipping 321 matching lines @@
 #endif
 }
 
 SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
     scoped_ptr<ClientSocketHandle> transport_socket,
     const HostPortPair& host_and_port,
     const SSLConfig& ssl_config,
     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),
       client_auth_cert_needed_(false),
       cert_verifier_(context.cert_verifier),
       server_bound_cert_service_(context.server_bound_cert_service),
       ssl_(NULL),
       transport_bio_(NULL),
       transport_(transport_socket.Pass()),
       host_and_port_(host_and_port),
@@ skipping 38 matching lines @@
   int rv = SSL_export_keying_material(
       ssl_, out, outlen, label.data(), label.size(),
       reinterpret_cast<const unsigned char*>(context.data()),
       context.length(), context.length() > 0);
 
   if (rv != 1) {
     int ssl_error = SSL_get_error(ssl_, rv);
     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;
 }
 
 int SSLClientSocketOpenSSL::GetTLSUniqueChannelBinding(std::string* out) {
   NOTIMPLEMENTED();
   return ERR_NOT_IMPLEMENTED;
 }
 
 int SSLClientSocketOpenSSL::Connect(const CompletionCallback& callback) {
@@ skipping 34 matching lines @@
   }
 
   // Shut down anything that may call us back.
   verifier_.reset();
   transport_->socket()->Disconnect();
 
   // Null all callbacks, delete all buffers.
   transport_send_busy_ = false;
   send_buffer_ = NULL;
   transport_recv_busy_ = false;
-  transport_recv_eof_ = false;
   recv_buffer_ = NULL;
 
   user_connect_callback_.Reset();
   user_read_callback_.Reset();
   user_write_callback_.Reset();
   user_read_buf_         = NULL;
   user_read_buf_len_     = 0;
   user_write_buf_        = NULL;
   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;
 
   cert_authorities_.clear();
   cert_key_types_.clear();
   client_auth_cert_needed_ = false;
 }
 
 bool SSLClientSocketOpenSSL::IsConnected() const {
@@ skipping 112 matching lines @@
       << SSLConnectionStatusToVersion(ssl_info->connection_status);
   return true;
 }
 
 int SSLClientSocketOpenSSL::Read(IOBuffer* buf,
                                  int buf_len,
                                  const CompletionCallback& callback) {
   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;
   } else {
     if (rv > 0)
       was_ever_used_ = true;
     user_read_buf_ = NULL;
     user_read_buf_len_ = 0;
   }
 
   return rv;
 }
 
 int SSLClientSocketOpenSSL::Write(IOBuffer* buf,
                                   int buf_len,
                                   const CompletionCallback& callback) {
   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;
   } else {
     if (rv > 0)
       was_ever_used_ = true;
     user_write_buf_ = NULL;
     user_write_buf_len_ = 0;
   }
 
@@ skipping 143 matching lines @@
 bool SSLClientSocketOpenSSL::DoTransportIO() {
   bool network_moved = false;
   int rv;
   // Read and write as much data as possible. The loop is necessary because
   // Write() may return synchronously.
   do {
     rv = BufferSend();
     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;
 }
 
 int SSLClientSocketOpenSSL::DoHandshake() {
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
   int net_error = OK;
   int rv = SSL_do_handshake(ssl_);
 
   if (client_auth_cert_needed_) {
@@ skipping 25 matching lines @@
                    base::Unretained(server_cert_.get())));
     GotoState(STATE_VERIFY_CERT);
   } else {
     int ssl_error = SSL_get_error(ssl_, rv);
 
     if (ssl_error == SSL_ERROR_WANT_CHANNEL_ID_LOOKUP) {
       // The server supports TLS channel id and the lookup is asynchronous.
       // 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
     if (net_error == ERR_IO_PENDING) {
       GotoState(STATE_HANDSHAKE);
     } else {
       LOG(ERROR) << "handshake failed; returned " << rv
                  << ", SSL error code " << ssl_error
                  << ", net_error " << net_error;
       net_log_.AddEvent(
@@ skipping 122 matching lines @@
     // In handshake phase.
     OnHandshakeIOComplete(result);
     return;
   }
 
   // Network layer received some data, check if client requested to read
   // decrypted data.
   if (!user_read_buf_.get())
     return;
 
-  int rv = DoReadLoop(result);
+  int rv = DoReadLoop();
   if (rv != ERR_IO_PENDING)
     DoReadCallback(rv);
 }
 
 int SSLClientSocketOpenSSL::DoHandshakeLoop(int last_io_result) {
   int rv = last_io_result;
   do {
     // Default to STATE_NONE for next state.
     // (This is a quirk carried over from the windows
     // implementation.  It makes reading the logs a bit harder.)
@@ skipping 23 matching lines @@
     if (network_moved && next_handshake_state_ == STATE_HANDSHAKE) {
       // In general we exit the loop if rv is ERR_IO_PENDING.  In this
       // special case we keep looping even if rv is ERR_IO_PENDING because
       // the transport IO may allow DoHandshake to make progress.
       rv = OK;  // This causes us to stay in the loop.
     }
   } while (rv != ERR_IO_PENDING && next_handshake_state_ != STATE_NONE);
   return rv;
 }
 
-int SSLClientSocketOpenSSL::DoReadLoop(int result) {
-  if (result < 0)
-    return result;
-
+int SSLClientSocketOpenSSL::DoReadLoop() {
   bool network_moved;
   int rv;
   do {
     rv = DoPayloadRead();
     network_moved = DoTransportIO();
   } while (rv == ERR_IO_PENDING && network_moved);
 
   return rv;
 }
 
-int SSLClientSocketOpenSSL::DoWriteLoop(int result) {
-  if (result < 0)
-    return result;
-
+int SSLClientSocketOpenSSL::DoWriteLoop() {
   bool network_moved;
   int rv;
   do {
     rv = DoPayloadWrite();
     network_moved = DoTransportIO();
   } while (rv == ERR_IO_PENDING && network_moved);
 
   return rv;
 }
 
@@ skipping 36 matching lines @@
     if (total_bytes_read > 0) {
       pending_read_error_ = rv;
       rv = total_bytes_read;
       next_result = &pending_read_error_;
     }
 
     if (client_auth_cert_needed_) {
       *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;
       }
     }
   }
 
   if (rv >= 0) {
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_RECEIVED, rv,
                                   user_read_buf_->data());
   }
   return rv;
 }
 
 int SSLClientSocketOpenSSL::DoPayloadWrite() {
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
   int rv = SSL_write(ssl_, user_write_buf_->data(), user_write_buf_len_);
 
   if (rv >= 0) {
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_SENT, rv,
                                   user_write_buf_->data());
     return rv;
   }
 
   int err = SSL_get_error(ssl_, rv);
-  return MapOpenSSLError(err, err_tracer);
+  return HandleOpenSSLError(err, err_tracer);
 }
 
 int SSLClientSocketOpenSSL::BufferSend(void) {
   if (transport_send_busy_)
     return ERR_IO_PENDING;
 
   if (!send_buffer_.get()) {
     // Get a fresh send buffer out of the send BIO.
     size_t max_read = BIO_ctrl_pending(transport_bio_);
     if (!max_read)
@@ skipping 65 matching lines @@
 
 void SSLClientSocketOpenSSL::BufferRecvComplete(int result) {
   result = TransportReadComplete(result);
   OnRecvComplete(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());
     send_buffer_->DidConsume(result);
     DCHECK_GE(send_buffer_->BytesRemaining(), 0);
     if (send_buffer_->BytesRemaining() <= 0)
       send_buffer_ = NULL;
   }
 }
 
 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);
     // A write into a memory BIO should always succeed.
     DCHECK_EQ(result, ret);
   }
   recv_buffer_ = NULL;
   transport_recv_busy_ = false;
   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) {
   DVLOG(3) << "OpenSSL ClientCertRequestCallback called";
   DCHECK(ssl == ssl_);
   DCHECK(*x509 == NULL);
   DCHECK(*pkey == NULL);
   if (!ssl_config_.send_client_cert) {
     // First pass: we know that a client certificate is needed, but we do not
     // have one at hand.
@@ skipping 158 matching lines @@
   DVLOG(2) << "next protocol: '" << npn_proto_ << "' status: " << npn_status_;
   return SSL_TLSEXT_ERR_OK;
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net