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),

[Ryan Sleevi, 2014/06/18 01:39:52]

Shouldn't this be ERR_IO_PENDING ?

[davidben, 2014/06/18 22:27:22]

Fixed docs.

       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;
+  }

[Ryan Sleevi, 2014/06/18 01:39:52]

No need for these braces.

[davidben, 2014/06/18 22:27:22]

Done. I think that was a remnant of when it was ERR_IO_PENDING and I had to wrap
the conditional.

   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_) {
     net_error = ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
@@ skipping 24 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;

[Ryan Sleevi, 2014/06/18 01:39:53]

I'm having trouble reasoning why this deletion is OK. I'm aware you said you
intentionally "pump the state", but it's unclear why this is desirable or
better.

That is, DoPayloadRead() will now fast-fail, but you're still going to force a
DoTransportIO call, and I don't know if that's actually a good thing, especially
if there's nothing to do.

[davidben, 2014/06/18 22:27:22]

Hrm. Good point that there's an extra DoTransportIO call. This is the thing I
mentioned I'll split out and/or drop in comment #7 so I'll leave it as is here
for context and we'll see what happens to the split-out CL.

(this bit:
)

   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) {
+  // Route transport failures around OpenSSL. Use |ssl_error| to determine

[Ryan Sleevi, 2014/06/18 01:39:52]

It's unclear what you mean by "around OpenSSL"

That is, one common, colloquial usage is "through" - eg: navigating it through
the various internal subsystems of OpenSSL.

Another is that you mean to "avoid" OpenSSL entirely.

It's further confusing through your last sentence, which suggests it's done "in
lieu of through", and what the reader can expect that might mean.

Put differently: Don't assume your reader is familiar with your alternative
designs (which I think you're hinting at using BIO to put an error on the
stack?) if you're going to mention them. At least provide a little more detail
:)

[davidben, 2014/06/18 22:27:22]

Rewrote comment.

+  // 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.

[Ryan Sleevi, 2014/06/18 01:39:53]

I think I'm getting confused by your actor model, which itself is confused
because of BIO.

"If OpenSSL wanted read data" really means "If the caller attempted to read
data", correct? If so, that latter phrasing makes it clearer, since the OpenSSL
model, three different things are reading - the caller from OpenSSL, OpenSSL
from the BIO, the BIO from the socket - and it's not clear at what phase you're
reporting this error.

[davidben, 2014/06/18 22:27:22]

I actually do mean if OpenSSL wanted to read data.

My model here is that the caller wants to read/write data from
SSLClientSocketOpenSSL, so we read/write from OpenSSL. In response to that,
OpenSSL may decide to read/write from OpenSSL's end of the BIO. Because of
renegotiations, OpenSSL's operations may not necessarily match ours. Reading
from OpenSSL's end from the BIO conceptually should then read from the
transport.

And then all these state machine complexities are to deal with us translating
back and forth between asynchronous + completion callback vs. non-blocking +
retry when ready.

It's kind of a mess because that last link---reading from BIO gets results from
reads on the transport---is managed by SSLClientSocketOpenSSL as BIO itself is a
BIO pair and gets data stuffed into it out-of-band. And also because there's no
good way to stuff error codes from the transport into the BIO.

This is all simulating what would have happened if we had errnos and used
SSL_ERROR_SYSCALL but does so without registering a BIO callback.

(Aaargh, all this terminology. It doesn't help that "write to the BIO" is
ambiguous because the crazy thing has two ends.)

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

Ok, hereby declaring a moratorium on the personification of OpenSSL :)

For clarity sake, let's refer to the actual reader of the BIO - the SSL* - so
that it's clear. If OpenSSL has some internal BIO doing things, then we call it
a BIO. etc.

Sound less confusing? :)

as for "write to the BIO", the term can be clarified by
"write outgoing ciphertext"
"write incoming socket data"

+    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, BIO_R_BROKEN_PIPE will be pushed on the error
+    // queue. Use this to signal transport write failure.

[Ryan Sleevi, 2014/06/18 01:39:52]

"On transport write failure, ... Use this to signal transport write failure" -
doesn't quite parse here either. I'm not sure what the rewording is, but more is
better.

"If the underlying transport socket was unable to write data, SSL_read/SSL_write
will add BIO_R_BROKEN_PIPE to the error queue. Use this to signal to the caller
that this socket is no longer writable"

[davidben, 2014/06/18 22:27:22]

Rephrased it a little.

+    //
+    // 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.

[Ryan Sleevi, 2014/06/18 01:39:52]

s/write or read/call to XXX or YYY/

[davidben, 2014/06/18 22:27:22]

Elaborated.

+    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