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 b63e120108eaba2cce38ebac3917f80501658c37..1dd474a9c73c234611e60b1865b50622b19534ef 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -329,9 +329,9 @@ 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_read_error_(OK),
       transport_write_error_(OK),
       server_cert_chain_(new PeerCertificateChain(NULL)),
       completed_handshake_(false),
@@ -445,7 +445,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();
@@ -457,6 +456,7 @@ void SSLClientSocketOpenSSL::Disconnect() {
   user_write_buf_len_    = 0;
 
   pending_read_error_ = kNoPendingReadResult;
+  transport_read_error_ = OK;
   transport_write_error_ = OK;
 
   server_cert_verify_result_.Reset();
@@ -659,6 +659,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));

[Ryan Sleevi, 2014/07/07 22:36:34]

Dang. So for now this is fine, but this would seem it would preclude the BIO
ever outliving the socket, right? I guess more of a note for havaard, though
that CL is stalled on BoringSSL figuring out the BIO semantics.

[davidben, 2014/07/08 00:03:34]

Hrm, that's true. If we ever need to cross that bridge, we'd want to move
transport_read_error_ and transport_write_error_ to the BIO's callback arg. Or
maybe just auxiliary data on the BIO with a custom BIO_METHOD.

This makes some sense since the equivalent fields are stored internally in the
nss_memio.

+
   SSL_set_bio(ssl_, ssl_bio, ssl_bio);
 
   // OpenSSL defaults some options to on, others to off. To avoid ambiguity,
@@ -777,7 +781,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_ != OK && BufferRecv() != ERR_IO_PENDING)
     network_moved = true;
   return network_moved;
 }
@@ -1261,20 +1265,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());
@@ -1287,19 +1280,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 MapOpenSSLError
+  // does not report success.
+  if (result == 0)
+    result = ERR_CONNECTION_CLOSED;

[Ryan Sleevi, 2014/07/07 22:36:34]

aside: It does seem unfortunate (read: potentially dangerous) that we treat an
incomplete transport failure as a graceful TLS failure.

[davidben, 2014/07/08 00:03:35]

Yeah, I was very surprised when that came up in the original iteration of this
code. We should have a DCHECK now though; OpenSSLPutNetError will NOTREACHED()
if you ever pass 0 to it.

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

[Ryan Sleevi, 2014/07/07 22:36:34]

This comment is no longer correct. <0 != 0

[davidben, 2014/07/08 00:03:34]

Done.

+    transport_read_error_ = result;
   } else {
     DCHECK(recv_buffer_.get());
     int ret = BIO_write(transport_bio_, recv_buffer_->data(), result);
@@ -1448,6 +1437,48 @@ 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_READ|BIO_CB_RETURN) && retvalue <= 0) {

[Ryan Sleevi, 2014/07/07 22:36:34]

Any sanity DCHECKs on argp/argi/argl needed?

[Ryan Sleevi, 2014/07/07 22:36:34]

if cmd == BIO_cb_return(BIO_CB_READ) ?

[davidben, 2014/07/08 00:03:35]

Hrm. That's been nuked in Boring. (Plus it isn't even used internally in
OpenSSL.)

[davidben, 2014/07/08 00:03:35]

Not especially I don't think? For BIO_CB_READ and BIO_CB_WRITE, argp ends up
being the buffer, argi the size of the buffer, and argl 0.

+    // If there is no more data in the buffer, report any pending errors that
+    // were observed. Note that both the readbuf and the writebuf are checked
+    // for errors, 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. See
+    // https://crbug.com/249848.
+    if (transport_read_error_ != OK) {
+      OpenSSLPutNetError(FROM_HERE, transport_read_error_);
+      return -1;
+    }
+    if (transport_write_error_ != OK) {
+      OpenSSLPutNetError(FROM_HERE, transport_write_error_);
+      return -1;
+    }
+  } else if (cmd == BIO_CB_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 to |bio|.
+    if (transport_write_error_ != OK) {
+      OpenSSLPutNetError(FROM_HERE, transport_write_error_);
+      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);

[Ryan Sleevi, 2014/07/07 22:36:34]

CHECK(socket) before hand?

Save us running off into the land of weirdness.

[davidben, 2014/07/08 00:03:35]

Done.

+}
+
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;