Eliminate ScopedOpenSSL in favour of scoped_ptr<> specializations.

Eliminate ScopedOpenSSL in favour of scoped_ptr<> specializations.

Match the NSS, CryptoAPI (Win) and Security (OS X) approaches by
declaring the scoped types as specializations of our existing scoped
classes.

Like NSS, this requires an intermediate helper type, because our
scoped_ptr<> doesn't accept deleter functions as template
arguments (though they are valid in C++11's unique_ptr<>). A few base
cryptographic (non-certificate) types are used in
scoped_openssl_types.h, while the remainder are left for
implementations to specialize as needed.

In an ideal world, this would be scoped_ptr<FOO, FOO_free>, but that
will require unique_ptr<> support.

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

[Ryan Sleevi, 2014-07-02 06:17:37]

eroman: net/ and content/ and chrome/, and if you're happy, I'll push to wtc for
crypto/

jyasskin: FYI. My feeling is that we may want to "fix" scoped_ptr<>, but that's
going to be a whole lot more template specialization logic (e.g. updating
type_traits to handle reference-types for the deleter via remove_reference,
turning the storage into a template with specializations so that we can
conditionally do the empty base class optimization, conditionally enabling some
of the functionality based on if D is a pointer). I'm not sure if this use case
is worth the specialization overheads though, given the prevalence of
scoped_ptr<>

[wtc, 2014-07-02 20:13:13]

Patch set 2 LGTM.

https://codereview.chromium.org/361193003/diff/20001/crypto/ec_signature_crea...
File crypto/ec_signature_creator_openssl.cc (right):

https://codereview.chromium.org/361193003/diff/20001/crypto/ec_signature_crea...
crypto/ec_signature_creator_openssl.cc:23: ScopedECDSA_SIG;

You can use the ScopedECDSA_SIG typedef from "crypto/scoped_openssl_types.h".

https://codereview.chromium.org/361193003/diff/20001/crypto/scoped_openssl_ty...
File crypto/scoped_openssl_types.h (right):

https://codereview.chromium.org/361193003/diff/20001/crypto/scoped_openssl_ty...
crypto/scoped_openssl_types.h:41: typedef scoped_ptr<RSA, OpenSSLDestroyer<RSA,
RSA_free> > ScopedRSA;

Nit: The criteria for which typedefs get to be defined here are not clear. It
seems that the goal is to minimize changes to some files, such as
net/android/keystore_unittest.cc.

https://codereview.chromium.org/361193003/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/361193003/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:221: friend struct
crypto::OpenSSLDestroyer<STACK_OF(X509), FreeX509Stack>;

This friend declaration seems wrong. To match the original code, it is
ScopedX509Stack that should be declared a friend.

[Ryan Sleevi, 2014-07-02 20:16:30]

https://codereview.chromium.org/361193003/diff/20001/crypto/scoped_openssl_ty...
File crypto/scoped_openssl_types.h (right):

https://codereview.chromium.org/361193003/diff/20001/crypto/scoped_openssl_ty...
crypto/scoped_openssl_types.h:41: typedef scoped_ptr<RSA, OpenSSLDestroyer<RSA,
RSA_free> > ScopedRSA;
On 2014/07/02 20:13:13, wtc wrote:
> 
> Nit: The criteria for which typedefs get to be defined here are not clear. It
> seems that the goal is to minimize changes to some files, such as
> net/android/keystore_unittest.cc.

Similar to ScopedNSS/ScopedCAPI, this only defines scoped helpers for primitive
types (eg: those that are not part of X.509/cert), keeping in line with
crypto/'s general agnosticism towards certs (which net/cert handles)

Does this comment work for you:
// Provide a few typedefs for common cryptographic primitives. Note that this
intentionally
// omits types related to certificates or other PKI technologies that crypto/
does not handle.

https://codereview.chromium.org/361193003/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/361193003/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:221: friend struct
crypto::OpenSSLDestroyer<STACK_OF(X509), FreeX509Stack>;
On 2014/07/02 20:13:13, wtc wrote:
> 
> This friend declaration seems wrong. To match the original code, it is
> ScopedX509Stack that should be declared a friend.

It's correct. OpenSSLDestroyer is what actually calls FreeX509Stack.
ScopedX509Stack just calls the Destroyer.

[wtc, 2014-07-02 20:22:00]

https://codereview.chromium.org/361193003/diff/20001/crypto/scoped_openssl_ty...
File crypto/scoped_openssl_types.h (right):

https://codereview.chromium.org/361193003/diff/20001/crypto/scoped_openssl_ty...
crypto/scoped_openssl_types.h:41: typedef scoped_ptr<RSA, OpenSSLDestroyer<RSA,
RSA_free> > ScopedRSA;

On 2014/07/02 20:16:29, Ryan Sleevi wrote:
>
> Does this comment work for you:
> // Provide a few typedefs for common cryptographic primitives. Note that this
> intentionally
> // omits types related to certificates or other PKI technologies that crypto/
> does not handle.

Yes. (I see that you already explained this criterion in the CL's description,
but it's good to have this comment in the code as well.)

The reason I wondered about this was that I saw a ScopedEVP_MD_CTX typedef but
not a ScopedEVP_CIPHER_CTX.

[Jeffrey Yasskin, 2014-07-02 20:22:34]

This will actually remain the right thing to do even in a unique_ptr world
because you don't want to have to remember to pass BIO_free_all to every
creation of a ScopedBIO. I haven't studied the patch in detail, but the parts I
looked at LGTM.

[Ryan Sleevi, 2014-07-02 20:50:48]

On 2014/07/02 20:22:34, Jeffrey Yasskin wrote:
> This will actually remain the right thing to do even in a unique_ptr world
> because you don't want to have to remember to pass BIO_free_all to every
> creation of a ScopedBIO. I haven't studied the patch in detail, but the parts
I
> looked at LGTM.

jyasskin: In C++11, you should be able to use "explicit unique_ptr(pointer p)"
to initialize, since D will be default-initialized. If D is a function-pointer
(eg: BIO_free_all), then default-initialization should succeed - indeed, that's
what we're relying on with the existing template.

So we shouldn't need the unique_ptr(pointer p, d1) ctor, unless I missed
something.

[Jeffrey Yasskin, 2014-07-02 21:05:23]

On 2014/07/02 20:50:48, Ryan Sleevi wrote:
> On 2014/07/02 20:22:34, Jeffrey Yasskin wrote:
> > This will actually remain the right thing to do even in a unique_ptr world
> > because you don't want to have to remember to pass BIO_free_all to every
> > creation of a ScopedBIO. I haven't studied the patch in detail, but the
parts
> I
> > looked at LGTM.
> 
> jyasskin: In C++11, you should be able to use "explicit unique_ptr(pointer p)"
> to initialize, since D will be default-initialized. If D is a function-pointer
> (eg: BIO_free_all), then default-initialization should succeed - indeed,
that's
> what we're relying on with the existing template.
> 
> So we shouldn't need the unique_ptr(pointer p, d1) ctor, unless I missed
> something.

D can only be a type, so it'd be something like void (*)(BIO *). The "explicit
unique_ptr(pointer p)" will value-initialize the deleter, which will result in a
null function pointer.

[eroman, 2014-07-07 21:32:20]

I find this new abstraction less readable than ScopedOpenSSL.

Why not keep a named type which binds the destroyer for the scoped_ptr<> so each
callsite doesn't need to pass it in?

My template-fu isn't the best, but the following works for me in my local build:

-------------------

template <typename Type, void (*Destroyer)(Type*)>
struct OpenSSLDestroyer {
  void operator()(Type* ptr) const {
    Destroyer(ptr);
  }
};

template <typename Type, void (*Destroyer)(Type*)>
class ScopedOpenSSL {
  public:
    typedef scoped_ptr<Type, OpenSSLDestroyer<Type, Destroyer> > T;
};

---------------------

Now the refactor is much closer to the original code, you just have to add an
additional ::T to the type:

ScopedOpenSSL<EVP_MD_CTX, EVP_MD_CTX_destroy> ctx(EVP_MD_CTX_create());

becomes:

ScopedOpenSSL<EVP_MD_CTX, EVP_MD_CTX_destroy>::T ctx(EVP_MD_CTX_create());

WDYT?

[Ryan Sleevi, 2014-07-07 21:40:00]

On 2014/07/07 21:32:20, eroman wrote:
> I find this new abstraction less readable than ScopedOpenSSL.

Could you elaborate on which parts you're concerned about, so that I can try to
address them?

> 
> Why not keep a named type which binds the destroyer for the scoped_ptr<> so
each
> callsite doesn't need to pass it in?
> 
> My template-fu isn't the best, but the following works for me in my local
build:
> 
> -------------------
> 
> template <typename Type, void (*Destroyer)(Type*)>
> struct OpenSSLDestroyer {
>   void operator()(Type* ptr) const {
>     Destroyer(ptr);
>   }
> };
> 
> template <typename Type, void (*Destroyer)(Type*)>
> class ScopedOpenSSL {
>   public:
>     typedef scoped_ptr<Type, OpenSSLDestroyer<Type, Destroyer> > T;
> };
> 
> ---------------------
> 
> Now the refactor is much closer to the original code, you just have to add an
> additional ::T to the type:
> 
> ScopedOpenSSL<EVP_MD_CTX, EVP_MD_CTX_destroy> ctx(EVP_MD_CTX_create());
> 
> becomes:
> 
> ScopedOpenSSL<EVP_MD_CTX, EVP_MD_CTX_destroy>::T ctx(EVP_MD_CTX_create());
> 
> WDYT?

I'm rather opposed to keeping ScopedOpenSSL around. I'm not actually fond of
ScopedBIO (and friends) either, but I kept them around both for compat and to
match our NSS types.

In general, I think these sorts of abstractions-around-abstractions are harmful
- case in point when ScopedOpenSSL wasn't made (or updated) to use scoped_ptr. I
also (generally) dislike trait types like that, since though I understand them,
I find it causes more people trouble with readability than they're generally
worth.

There may be some template tricks that I can likely do, to convert to:
scoped_ptr<EVP_MD_CTX, FunctionDeleter<EVP_MD_CTX_destroy> > (or something
similar)

but I do want to make sure that at the call site, it's visible to developers
that this is a scoped_ptr<>, not one of our many not-quite scoped wrappers (eg:
the COM bits, HANDLEs, etc)

[eroman, 2014-07-07 21:56:20]

The inconsistency is in where verbosity is eliminated.

Things like ScopedBIO already hide the fact that it is a scoped_ptr<> under the
hood. Whereas for the non-blessed types, files need to explicitly specify the
openssl destroyer wrapper and duplicate the type in nested template.

If you are going to optimize one for verbosity, why not the other?

LGTM.

https://codereview.chromium.org/361193003/diff/20001/crypto/scoped_openssl_ty...
File crypto/scoped_openssl_types.h (right):

https://codereview.chromium.org/361193003/diff/20001/crypto/scoped_openssl_ty...
crypto/scoped_openssl_types.h:25: if (ptr)
no need for this test, scoped_ptr<> only calls it when non-null.

[wtc, 2014-07-09 15:08:15]

https://codereview.chromium.org/361193003/diff/20001/chrome/common/net/x509_c...
File chrome/common/net/x509_certificate_model_openssl.cc (right):

https://codereview.chromium.org/361193003/diff/20001/chrome/common/net/x509_c...
chrome/common/net/x509_certificate_model_openssl.cc:461:
crypto::OpenSSLDestroyer<ASN1_BIT_STRING, ASN1_BIT_STRING_free> >

I also find the new code less readable because of the nested templates and the
duplicated OpenSSL type. Perhaps typedefs can be used more. Because of the
consistent naming convention of the free functions, a macro can also be used to
simplify this. I remember OpenSSL uses macros to build these free functions, so
the consistent naming convention is guaranteed. On the other hand, OpenSSL's use
of those macros seems to be a common complaint from people who need to delve
into OpenSSL source code.

[Ryan Sleevi, 2014-07-10 01:31:19]

eroman: After playing a bit more with the templates, and being depressed by
everything, I ended up going with a variation on your solution.

jyasskin: FWIW, NSS also has done an approach like this (to wrap NSS C API
calls). The tricks like std::function() have the overhead of requiring call-time
specification of the pointer, versus template constant (like done here), so I
suspect as/if we scoped_ptr/unique_ptr other C-APIs, we'll find ourselves doing
this. But maybe not.

[eroman, 2014-07-10 01:38:59]

lgtm

https://codereview.chromium.org/361193003/diff/40001/crypto/scoped_openssl_ty...
File crypto/scoped_openssl_types.h (right):

https://codereview.chromium.org/361193003/diff/40001/crypto/scoped_openssl_ty...
crypto/scoped_openssl_types.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
2014

[Ryan Sleevi, 2014-07-10 01:57:41]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2014-07-10 01:59:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsleevi@chromium.org/361193003/80001

[commit-bot: I haz the power, 2014-07-10 04:39:44]

Change committed as 282257