Remove manual CRYPTO_add calls.

Remove manual CRYPTO_add calls.

Use X509_chain_up_ref, EVP_PKEY_dup, and newly-added X509_up_ref
instead. Also RAII OpenSSLClientKeyStore.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289498

[Ryan Sleevi, 2014-08-13 22:49:39]

Adding dcheng@ as an FYI on this gotcha, but if you feel so motivated, you could
probably send a fix ajwong's way :)

https://codereview.chromium.org/474663002/diff/1/net/ssl/openssl_client_key_s...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/474663002/diff/1/net/ssl/openssl_client_key_s...
net/ssl/openssl_client_key_store.cc:55:
private_key.reset(EVP_PKEY_dup(other.private_key.get()));
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]

https://codereview.chromium.org/474663002/diff/1/net/ssl/openssl_client_key_s...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/474663002/diff/1/net/ssl/openssl_client_key_s...
net/ssl/openssl_client_key_store.cc:55:
private_key.reset(EVP_PKEY_dup(other.private_key.get()));
On 2014/08/13 22:49:38, Ryan Sleevi wrote:
> 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".

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.

[Ryan Sleevi, 2014-08-14 00:37:52]

On 2014/08/13 23:41:39, David Benjamin wrote:
>
https://codereview.chromium.org/474663002/diff/1/net/ssl/openssl_client_key_s...
> File net/ssl/openssl_client_key_store.cc (right):
> 
>
https://codereview.chromium.org/474663002/diff/1/net/ssl/openssl_client_key_s...
> net/ssl/openssl_client_key_store.cc:55:
> private_key.reset(EVP_PKEY_dup(other.private_key.get()));
> On 2014/08/13 22:49:38, Ryan Sleevi wrote:
> > 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".
> 
> 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.

I'm not sure your comment about std::vector - it's not an issue for
std::unique_ptr<>, it's just an implementation detail of ours that we never
changed.

[Ryan Sleevi, 2014-08-14 00:38:26]

lgtm

[davidben, 2014-08-14 00:48:43]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-08-14 00:55:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/474663002/40001

[commit-bot: I haz the power, 2014-08-14 09:31:09]

Committed patchset #3 (40001) as 289498