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..41da340ac320ba01ae9f2a06997951a1cc543f11 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),

[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),
@@ -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,8 +785,9 @@ 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;
+  }

[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;
 }
 
@@ -831,7 +832,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 +975,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 +1019,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;

[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 {
@@ -1032,10 +1030,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 +1087,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 +1119,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 +1205,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 +1223,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 +1243,43 @@ 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| 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) {