Close SSLClientSocketOpenSSL cleanly if the transport was closed.

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 <errno.h>
@@ skipping 1328 matching lines @@
     // Otherwise, an error occurred (rv <= 0). The error needs to be handled
     // immediately, while the OpenSSL errors are still available in
     // thread-local storage. However, the handled/remapped error code should
     // only be returned if no application data was already read; if it was, the
     // error code should be deferred until the next call of DoPayloadRead.
     //
     // If no data was read, |*next_result| will point to the return value of
     // this function. If at least some data was read, |*next_result| will point
     // to |pending_read_error_|, to be returned in a future call to
     // DoPayloadRead() (e.g.: after the current data is handled).
     int *next_result = &rv;

[davidben, 2014/10/14 20:31:06]

This logic (and similar logic in SSLClientSocketNSS) is incredibly convoluted
and next_result switches from type openssl_return_t* to net_return_t* part-way
through.  I'm going to leave cleaning that up to a follow-up so this can be
merged. It's orthogonal to this change.

     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);
+
+      // Servers do not reliably send close_notify, so
+      // ERR_CONNECTION_CLOSED cannot be treated as fatal.

[Ryan Sleevi, 2014/10/17 20:53:00]

This comment needs explaining (or my flu meds need to not be as strong). Namely,
you're relying on a lot of internal context in your head for this comment that
is lost in the surrounding code :)

// Many servers do not reliably send a close_notify alert when shutting
// down a connection, and instead terminate the TCP connection. This
// is reported as ERR_CONNECTION_CLOSED. Because of this, map the
// unclean shutdown to a graceful EOF, instead of treating it as an
// error as it should be.

[davidben, 2014/10/20 17:13:24]

Done.

+      if (*next_result == ERR_CONNECTION_CLOSED)
+        *next_result = 0;
+
       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.
         *next_result = kNoPendingReadResult;
       }
@@ skipping 113 matching lines @@
     if (send_buffer_->BytesRemaining() <= 0)
       send_buffer_ = NULL;
   }
 }
 
 int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   DCHECK(ERR_IO_PENDING != result);
   // If an EOF, canonicalize to ERR_CONNECTION_CLOSED here so MapOpenSSLError
   // does not report success.
   if (result == 0)
     result = ERR_CONNECTION_CLOSED;

[davidben, 2014/10/14 20:31:06]

It is a little annoying that this fix is undoing this canonicalization. The
awkward thing is that we /do/ want ERR_CONNECTION_CLOSED if it shuts off during
the handshake, but we /don't/ want it if it shuts off during a read.

Another option would be to:

- Remove this mapping. Instead, transport_read_error_ now stores 0 for EOF and
we pick some other value (ERR_IO_PENDING?) to signal that it's still readable.
Probably want to do the same thing to transport_write_error_. Then arrange for
BIO_read of the SSL* end of transport_bio_ to return 0 if the underlying
transport was closed, and -1 + OpenSSLPutNetError on error. This could either by
done by calling SSL_shutdown_wr or adjusting the callback. Either way,
transport_read_error_ needs to be adjusted for the BufferRecv() guard in line
879.

- This would make the SSL_read code DTRT because we'd get a zero return.
SSL_get_error would return SSL_ERROR_ZERO_RETURN or SSL_ERROR_SYSCALL if we
cared to distinguish close_notify but we don't.

- However, getting SSL_do_handshake right becomes tricky. (Before the original
CL, we didn't bother because everthing turned into ERR_SSL_PROTOCOL_ERROR.)
OpenSSL just bubbles the BIO return code up if it's 0 or -1. BUT it also returns
0 if you get a close_notify during the handshake, which should be
ERR_SSL_PROTOCOL_ERROR. The distinguishing factor is one gives
SSL_ERROR_ZERO_RETURN in SSL_get_error and the other SSL_ERROR_SYSCALL. So I
believe we need to first SSL_get_error, special-case rv == 0 && err ==
SSL_ERROR_SYSCALL to ERR_CONNECTION_CLOSED, and then do the normal thing.

This is all kind of a mess and it seems plumbing EOF through the error codepath
and mapping back in Read is simpler, but I can do the other one if you prefer.

[Ryan Sleevi, 2014/10/17 20:53:00]

Sorry, I have trouble following you. It might be easier to see in code, but it
sounds... hackier?
DTRT?

[davidben, 2014/10/20 17:13:24]

I think I agree.
Do The Right Thing. Sorry, I keep forgetting which acronyms are MIT slang and
which are Google slang. :-)

   if (result < 0) {
     DVLOG(1) << "TransportReadComplete result " << result;
     // Received 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);
@@ skipping 261 matching lines @@
                                             ct::SCT_STATUS_LOG_UNKNOWN));
   }
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net