This CL corrects a bug in which the OnHandshakeComplete callback for an ssl session was never called

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 345 matching lines @@
       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),
       trying_cached_session_(false),
       next_handshake_state_(STATE_NONE),
       npn_status_(kNextProtoUnsupported),
       channel_id_xtn_negotiated_(false),
+      session_completion_count_(0),
       net_log_(transport_->socket()->NetLog()) {
 }
 
 SSLClientSocketOpenSSL::~SSLClientSocketOpenSSL() {
   Disconnect();
 }
 
 bool SSLClientSocketOpenSSL::InSessionCache() const {
   SSLContext* context = SSLContext::GetInstance();
   std::string cache_key = GetSessionCacheKey();
@@ skipping 52 matching lines @@
 int SSLClientSocketOpenSSL::Connect(const CompletionCallback& callback) {
   net_log_.BeginEvent(NetLog::TYPE_SSL_CONNECT);
 
   // Set up new ssl object.
   int rv = Init();
   if (rv != OK) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
     return rv;
   }
 
-  if (!handshake_completion_callback_.is_null()) {
-    SSLContext* context = SSLContext::GetInstance();
-    context->session_cache()->SetSessionAddedCallback(
-        ssl_,
-        base::Bind(&SSLClientSocketOpenSSL::OnHandshakeCompletion,
-                   base::Unretained(this)));
-  }
-
   // Set SSL to client mode. Handshake happens in the loop below.
   SSL_set_connect_state(ssl_);
 
   GotoState(STATE_HANDSHAKE);
   rv = DoHandshakeLoop(OK);
   if (rv == ERR_IO_PENDING) {
     user_connect_callback_ = callback;
   } else {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
     if (rv < OK)
       OnHandshakeCompletion();
   }
 
   return rv > OK ? OK : rv;
 }
 
 void SSLClientSocketOpenSSL::Disconnect() {
   // If a handshake was pending (Connect() had been called), notify interested
   // parties that it's been aborted now. If the handshake had already
   // completed, this is a no-op.
   OnHandshakeCompletion();
   if (ssl_) {
-    SSLContext* context = SSLContext::GetInstance();
-    context->session_cache()->RemoveSessionAddedCallback(ssl_);
     // Calling SSL_shutdown prevents the session from being marked as
     // unresumable.
     SSL_shutdown(ssl_);
     SSL_free(ssl_);
     ssl_ = NULL;
   }
   if (transport_bio_) {
     BIO_free_all(transport_bio_);
     transport_bio_ = NULL;
   }
@@ skipping 200 matching lines @@
 }
 
 int SSLClientSocketOpenSSL::SetReceiveBufferSize(int32 size) {
   return transport_->socket()->SetReceiveBufferSize(size);
 }
 
 int SSLClientSocketOpenSSL::SetSendBufferSize(int32 size) {
   return transport_->socket()->SetSendBufferSize(size);
 }
 
+// static
+void SSLClientSocketOpenSSL::OnSessionFinishedCallback(const SSL* ssl,
+                                                       int result,
+                                                       int unused) {
+  SSLClientSocketOpenSSL* ssl_socket =
+      SSLContext::GetInstance()->GetClientSocketFromSSL(ssl);
+  if (result == SSL_CB_HANDSHAKE_DONE)
+    ssl_socket->CheckIfSessionFinished();
+}
+
 int SSLClientSocketOpenSSL::Init() {
   DCHECK(!ssl_);
   DCHECK(!transport_bio_);
 
   SSLContext* context = SSLContext::GetInstance();
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
 
   ssl_ = SSL_new(context->ssl_ctx());
   if (!ssl_ || !context->SetClientSocketForSSL(ssl_, this))
     return ERR_UNEXPECTED;
 
   if (!SSL_set_tlsext_host_name(ssl_, host_and_port_.host().c_str()))
     return ERR_UNEXPECTED;
 
+  // Set an OpenSSL callback to monitor this SSL*'s connection.
+  SSL_set_info_callback(ssl_, &OnSessionFinishedCallback);
+
   trying_cached_session_ = context->session_cache()->SetSSLSessionWithKey(
       ssl_, GetSessionCacheKey());
 
   BIO* ssl_bio = NULL;
   // 0 => use default buffer sizes.
   if (!BIO_new_bio_pair(&ssl_bio, 0, &transport_bio_, 0))
     return ERR_UNEXPECTED;
   DCHECK(ssl_bio);
   DCHECK(transport_bio_);
 
@@ skipping 311 matching lines @@
       net_log_);
 }
 
 int SSLClientSocketOpenSSL::DoVerifyCertComplete(int result) {
   verifier_.reset();
 
   if (result == OK) {
     // TODO(joth): Work out if we need to remember the intermediate CA certs
     // when the server sends them to us, and do so here.
     SSLContext::GetInstance()->session_cache()->MarkSSLSessionAsGood(ssl_);
+    CheckIfSessionFinished();
   } else {
     DVLOG(1) << "DoVerifyCertComplete error " << ErrorToString(result)
              << " (" << result << ")";
   }
 
   completed_handshake_ = true;
   // Exit DoHandshakeLoop and return the result to the caller to Connect.
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   return result;
 }
@@ skipping 525 matching lines @@
     // 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;
 }
 
+// Determines if the session for |ssl| is in the cache, and calls the
+// appropriate callback if that is the case.
+//
+// CheckIfSessionFinished is called twice per connection: once after
+// MarkSSLSessionAsGood, when the certificate has been verified, and
+// once via an OpenSSL callback when the handshake has completed. On the
+// second call, the connection's handshake completion callback is run.
+void SSLClientSocketOpenSSL::CheckIfSessionFinished() {
+  // Increment the session's completion count.
+  if (++session_completion_count_ == 2) {

[Ryan Sleevi, 2014/08/06 01:31:30]

Is this still necessary / the right approach, now that you're within the socket?

It seems like you have access to all the information you need internally, and
can make this more explicit. For example, rather than this count (which has
implicit state), you could set a bool when the cb was called.

if (cb_called_ &&
SSLContext::GetInstance()->session_cache()->SessionIsGood(ssl_))
  ...

[mshelley, 2014/08/06 03:09:31]

Done.

+    OnHandshakeCompletion();
+  }
+}
+
 // static
 long SSLClientSocketOpenSSL::BIOCallback(
     BIO *bio,
     int cmd,
     const char *argp, int argi, long argl,
     long retvalue) {
   SSLClientSocketOpenSSL* socket = reinterpret_cast<SSLClientSocketOpenSSL*>(
       BIO_get_callback_arg(bio));
   CHECK(socket);
   return socket->MaybeReplayTransportError(
       bio, cmd, argp, argi, argl, retvalue);
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net