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

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 3925ca1a36ec6b401d4625bb7d0055c308ab0943..8735dc725a68ffcc9cdac240dac97aeca8f5f26c 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -343,6 +343,7 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       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),
@@ -1204,7 +1205,7 @@ int SSLClientSocketOpenSSL::BufferRecv(void) {
   if (rv == ERR_IO_PENDING) {
     transport_recv_busy_ = true;
   } else {
-    TransportReadComplete(rv);
+    rv = TransportReadComplete(rv);
   }
   return rv;
 }
@@ -1216,7 +1217,7 @@ void SSLClientSocketOpenSSL::BufferSendComplete(int result) {
 }
 
 void SSLClientSocketOpenSSL::BufferRecvComplete(int result) {
-  TransportReadComplete(result);
+  result = TransportReadComplete(result);
   OnRecvComplete(result);
 }
 
@@ -1224,9 +1225,19 @@ 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());
@@ -1237,7 +1248,7 @@ void SSLClientSocketOpenSSL::TransportWriteComplete(int result) {
   }
 }
 
-void SSLClientSocketOpenSSL::TransportReadComplete(int result) {
+int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   DCHECK(ERR_IO_PENDING != result);
   if (result <= 0) {
     DVLOG(1) << "TransportReadComplete result " << result;
@@ -1246,8 +1257,11 @@ void SSLClientSocketOpenSSL::TransportReadComplete(int result) {
     // 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);
@@ -1259,6 +1273,7 @@ void SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   }
   recv_buffer_ = NULL;
   transport_recv_busy_ = false;
+  return result;
 }
 
 int SSLClientSocketOpenSSL::ClientCertRequestCallback(SSL* ssl,