Remove manual CRYPTO_add calls.

net/ssl/openssl_client_key_store.cc

 // Copyright (c) 2013 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.
 
 #include "net/ssl/openssl_client_key_store.h"
 
 #include <openssl/evp.h>
 #include <openssl/x509.h>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
 #include "net/cert/x509_certificate.h"
 
 namespace net {
 
 namespace {
 
-// Increment the reference count of a given EVP_PKEY. This function
-// is similar to EVP_PKEY_dup which is not available from the OpenSSL
-// version used by Chromium at the moment. Its name is distinct to
-// avoid compiler warnings about ambiguous function calls at caller
-// sites.
-EVP_PKEY* CopyEVP_PKEY(EVP_PKEY* key) {
-  if (key)
-    CRYPTO_add(&key->references, 1, CRYPTO_LOCK_EVP_PKEY);
-  return key;
-}
-
 // Return the EVP_PKEY holding the public key of a given certificate.
 // |cert| is a certificate.
 // Returns a scoped EVP_PKEY for it.
 crypto::ScopedEVP_PKEY GetOpenSSLPublicKey(const X509Certificate* cert) {
   // X509_PUBKEY_get() increments the reference count of its result.
   // Unlike X509_get_X509_PUBKEY() which simply returns a direct pointer.
   EVP_PKEY* pkey =
       X509_PUBKEY_get(X509_get_X509_PUBKEY(cert->os_cert_handle()));
   if (!pkey)
     LOG(ERROR) << "Can't extract private key from certificate!";
   return crypto::ScopedEVP_PKEY(pkey);
 }
 
 }  // namespace
 
 OpenSSLClientKeyStore::OpenSSLClientKeyStore() {
 }
 
 OpenSSLClientKeyStore::~OpenSSLClientKeyStore() {
 }
 
 OpenSSLClientKeyStore::KeyPair::KeyPair(EVP_PKEY* pub_key,
-                                        EVP_PKEY* priv_key) {
-  public_key = CopyEVP_PKEY(pub_key);
-  private_key = CopyEVP_PKEY(priv_key);
+                                        EVP_PKEY* priv_key)
+    : public_key(EVP_PKEY_dup(pub_key)),
+      private_key(EVP_PKEY_dup(priv_key)) {
 }
 
 OpenSSLClientKeyStore::KeyPair::~KeyPair() {
-  EVP_PKEY_free(public_key);
-  EVP_PKEY_free(private_key);
 }
 
-OpenSSLClientKeyStore::KeyPair::KeyPair(const KeyPair& other) {
-  public_key = CopyEVP_PKEY(other.public_key);
-  private_key = CopyEVP_PKEY(other.private_key);
+OpenSSLClientKeyStore::KeyPair::KeyPair(const KeyPair& other)
+    : public_key(EVP_PKEY_dup(other.public_key.get())),
+      private_key(EVP_PKEY_dup(other.private_key.get())) {
 }
 
 void OpenSSLClientKeyStore::KeyPair::operator=(const KeyPair& other) {
-  EVP_PKEY* old_public_key = public_key;
-  EVP_PKEY* old_private_key = private_key;
-  public_key = CopyEVP_PKEY(other.public_key);
-  private_key = CopyEVP_PKEY(other.private_key);
-  EVP_PKEY_free(old_private_key);
-  EVP_PKEY_free(old_public_key);
+  public_key.reset(EVP_PKEY_dup(other.public_key.get()));
+  private_key.reset(EVP_PKEY_dup(other.private_key.get()));

[Ryan Sleevi, 2014/08/13 22:49:38]

Are you sure this is safe w/r/t the scoped_ptr issue that eroman and mattm ran
into?

For a rather long-winded reason, but the TL;DR is "to catch dangerous behaviour
when transitioning", scoped_ptr<> will assert that ptr_ != T on a .reset(). If
|other| and |this| point to the same keypair, though this is safe (because
they're ref-counted), this will blow up.

Instead
public_key.reset();
public_key.reset(...);
private_key.reset();
private_key.reset(...);

See
http://src.chromium.org/viewvc/chrome/trunk/src/base/memory/scoped_ptr.h?r1=1...
/ https://code.google.com/p/chromium/issues/detail?id=162971

We took a solution NOT required by unique_ptr<>, but is more conservative, hence
this "surprise".

[davidben, 2014/08/13 23:41:39]

Mrrrf. That's obnoxious. I guess whether it's fine or not would depend on the
behavior of std::vector. Perhaps that should be guarded by a trait or additional
parameter... :-/

Made some temporaries.

 }
 
 int OpenSSLClientKeyStore::FindKeyPairIndex(EVP_PKEY* public_key) {
   if (!public_key)
     return -1;
   for (size_t n = 0; n < pairs_.size(); ++n) {
-    if (EVP_PKEY_cmp(pairs_[n].public_key, public_key) == 1)
+    if (EVP_PKEY_cmp(pairs_[n].public_key.get(), public_key) == 1)
       return static_cast<int>(n);
   }
   return -1;
 }
 
 void OpenSSLClientKeyStore::AddKeyPair(EVP_PKEY* pub_key,
                                        EVP_PKEY* private_key) {
   int index = FindKeyPairIndex(pub_key);
   if (index < 0)
     pairs_.push_back(KeyPair(pub_key, private_key));
@@ skipping 23 matching lines @@
     return crypto::ScopedEVP_PKEY();
 
   crypto::ScopedEVP_PKEY pub_key(GetOpenSSLPublicKey(client_cert));
   if (!pub_key.get())
     return crypto::ScopedEVP_PKEY();
 
   int index = FindKeyPairIndex(pub_key.get());
   if (index < 0)
     return crypto::ScopedEVP_PKEY();
 
-  return crypto::ScopedEVP_PKEY(CopyEVP_PKEY(pairs_[index].private_key));
+  return crypto::ScopedEVP_PKEY(EVP_PKEY_dup(pairs_[index].private_key.get()));
 }
 
 void OpenSSLClientKeyStore::Flush() {
   pairs_.clear();
 }
 
 OpenSSLClientKeyStore* OpenSSLClientKeyStore::GetInstance() {
   return Singleton<OpenSSLClientKeyStore>::get();
 }
 
 }  // namespace net