Close the correct end of the BIO pair on transport send failures in openssl.

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 325 matching lines @@
 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),
       completed_handshake_(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),
       ssl_config_(ssl_config),
       ssl_session_cache_shard_(context.ssl_session_cache_shard),
@@ skipping 94 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;
 

[Ryan Sleevi, 2014/02/04 20:45:02]

Don't you need to update transport_write_error_ here, lest it persist to a
reconnect?

[davidben, 2014/02/04 23:00:11]

Done. Also reset pending_read_error_.

   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;
 
   server_cert_verify_result_.Reset();
   completed_handshake_ = false;
@@ skipping 726 matching lines @@
 
   recv_buffer_ = new IOBuffer(max_write);
   int rv = transport_->socket()->Read(
       recv_buffer_.get(),
       max_write,
       base::Bind(&SSLClientSocketOpenSSL::BufferRecvComplete,
                  base::Unretained(this)));
   if (rv == ERR_IO_PENDING) {
     transport_recv_busy_ = true;
   } else {
-    TransportReadComplete(rv);
+    rv = TransportReadComplete(rv);
   }
   return rv;
 }
 
 void SSLClientSocketOpenSSL::BufferSendComplete(int result) {
   transport_send_busy_ = false;
   TransportWriteComplete(result);
   OnSendComplete(result);
 }
 
 void SSLClientSocketOpenSSL::BufferRecvComplete(int result) {
-  TransportReadComplete(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_));

[davidben, 2014/02/04 19:27:57]

Could get away with even avoiding this line for minimal fix purposes, but then
the test needs more openssl/nss special cases because transport write errors
don't actually translate into SSLClientSocketOpenSSL::Write failures right now.
(Specifically I need to remove the assertion that the buffer wasn't entirely
consumed, because it will be.)

[davidben, 2014/02/04 19:27:57]

It looks like sometimes OpenSSL wraps the wbio in a "bbio" for buffering or
something. So it might be better to keep a non-owning reference to ssl_bio and
shutdown that. But buffer_ctrl forwards to the downstream BIO, so I think this
works the same.

[Ryan Sleevi, 2014/02/04 20:45:02]

Future work: Should we be using BIO_set_close() instead?

[davidben, 2014/02/04 23:00:11]

Hrm. That seems to set the shutdown flag on the BIO. It doesn't look like that
affects reads from the other end? Docs say:

"""
BIO_set_close() sets the BIO b close flag to flag. flag can take the value
BIO_CLOSE or BIO_NOCLOSE. Typically BIO_CLOSE is used in a source/sink BIO to
indicate that the underlying I/O stream should be closed when the BIO is freed.
"""

which seems like something else?

+
+    // 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.
+    transport_write_error_ = result;
     (void)BIO_shutdown_wr(transport_bio_);
-    BIO_set_mem_eof_return(transport_bio_, 0);

[davidben, 2014/02/04 19:27:57]

Looks like these calls don't do anything in BIO pairs.

     send_buffer_ = NULL;

[davidben, 2014/02/04 19:27:57]

This line is sort of funny. The next time we get around to calling BufferSend,
it thinks there's nothing to write and BufferSend() actually succeeds with 0
bytes written. That should probably go away later to mirror the NSS logic; I
believe nss_memio.c does not advance the buffer until memio_PutWriteResult goes
through.

[Ryan Sleevi, 2014/02/04 20:45:02]

I'm not sure I follow why it's funny. We just had a transport write failure, why
shouldn't it nuke the send_buffer_?

The succeeds with zero case is odd though.

[davidben, 2014/02/04 23:00:11]

Keeping send_buffer_ around avoids the succeeds with zero case. The send buffer
itself is useless, but then we'd want other state somewhere to ensure
BufferSend() doesn't try to write the next bunch of data to the network the next
time it's called. The NSS code doesn't have such state, but it does the moral
equivalent of keeping send_buffer_ around. It doesn't advance the memio read
head until those bytes have successfully been written to the transport socket.

In the OpenSSL implementation, our transport_bio_ advances the read head
immediately and copies the data out into a separate buffer to be sent over the
network. So we either want to keep send_buffer_ around so the next BufferSend
will re-try sending over the transport socket and hit an error again, or nuke
send_buffer_ and save the socket error. Then have BufferSend not even touch the
transport socket in that case and just replay the error.

   } else {
     DCHECK(send_buffer_.get());
     send_buffer_->DidConsume(result);
     DCHECK_GE(send_buffer_->BytesRemaining(), 0);
     if (send_buffer_->BytesRemaining() <= 0)
       send_buffer_ = NULL;
   }
 }
 
-void SSLClientSocketOpenSSL::TransportReadComplete(int result) {
+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;
-    BIO_set_mem_eof_return(transport_bio_, 0);
     (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.
+    result = transport_write_error_;

[Ryan Sleevi, 2014/02/04 20:45:02]

Let's add bug numbers. This is perhaps the most confusing/concerning. It should
only cause a read failure if the overall Read() request caused a Write().

I'm somewhat concerned because we may have buffered up to 16K of data in the
local buffer, and that is data that *could* be read, *unless* Read() needed to
write data (eg: a renegotiation request, a Close alert, a Change Cipher Spec)

[davidben, 2014/02/04 23:00:11]

Agreed. I kept it this way just to minimize behavior change for the M33 merge,
but I don't think this is a good fix for trunk. Effectively all it's doing is
bypassing the CHECK and preserving the error for this one case.

That buffering bit is what I meant that the write failure => read shut-off
behavior in SSLClientSocketOpenSSL is different from SSLClientSocketNSS. Hence:

"""
This same loop and behavior exists in the OpenSSL logic but, because of the
wrong end of the BIO pair getting closed, it already had the NSS fix. (Mostly;
the NSS fix doesn't preclude adding bytes to the memio after
memio_PutWriteResult has been called with an error and it will first consume the
buffer before returning the error, while the BIO pair shutdown logic precludes
this behavior. That behavior is a little bizarre anyway though since it means
memio gets in a state where it will return data if there happens to be some
buffered, but echo the write error otherwise.)
"""

We could probably mimic the NSS behavior by installing a BIO_CB_WRITE callback
that inspects the size of the buffer. But if we're not doing that as part of the
33 fix, we probably should resolve the read/failed-write confusing better
altogether.

Added a reference to the bug number.

   } else {
     DCHECK(recv_buffer_.get());
     int ret = BIO_write(transport_bio_, recv_buffer_->data(), result);
     // A write into a memory BIO should always succeed.
     // Force values on the stack for http://crbug.com/335557
     base::debug::Alias(&result);
     base::debug::Alias(&ret);
     CHECK_EQ(result, ret);
   }
   recv_buffer_ = NULL;
   transport_recv_busy_ = false;
+  return result;
 }
 
 int SSLClientSocketOpenSSL::ClientCertRequestCallback(SSL* ssl,
                                                       X509** x509,
                                                       EVP_PKEY** pkey) {
   DVLOG(3) << "OpenSSL ClientCertRequestCallback called";
   DCHECK(ssl == ssl_);
   DCHECK(*x509 == NULL);
   DCHECK(*pkey == NULL);
 
@@ skipping 124 matching lines @@
   }
 
   npn_proto_.assign(reinterpret_cast<const char*>(*out), *outlen);
   server_protos_.assign(reinterpret_cast<const char*>(in), inlen);
   DVLOG(2) << "next protocol: '" << npn_proto_ << "' status: " << npn_status_;
 #endif
   return SSL_TLSEXT_ERR_OK;
 }
 
 }  // namespace net