Make X509Certificate::CreateFromHandle() copy the OSCertHandle, rather than assume ownership

Make X509Certificate::CreateFromHandle() copy the OSCertHandle, rather than assume ownership

R=wtc
BUG=47463
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50938

[Ryan Sleevi, 2010-06-25 03:40:20]

Follow-up from http://codereview.chromium.org/2809024/show

On 2010/06/24 02:33:46, wtc wrote:
> I find it confusing that the X509Certificate constructor takes over the
> ownership of cert_handle, but copies intermediates.  We should change it
> (in a separate CL) to also copy cert_handle.  This requires changing all
> the callers, so it'll be a lot of work, but it is worth doing.

Turns out it wasn't too big a deal. The places where we actually dealt with
creating raw OSCertHandles seems constrained to unit tests and the client
sockets.

[wtc, 2010-06-26 17:06:22]

LGTM, except the problem marked with "ISSUE" below in
x509_certificate_mac.cc.

Some of the FreeOSCertHandle calls we have to add make the
code look a little awkward.  Adding a ScopedOSCertHandle type
might help...

Note: I am on vacation next week and may not be able to review
new versions of this CL.  I've asked eroman to cover for me.

http://codereview.chromium.org/2867026/diff/1/3
File net/base/cert_test_util.cc (right):

http://codereview.chromium.org/2867026/diff/1/3#newcode62
net/base/cert_test_util.cc:62: X509Certificate::FreeOSCertHandle(cert);
Call CERT_DestroyCertificate(cert) here for consistency
with lines 47 and 54 above.

http://codereview.chromium.org/2867026/diff/1/4
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/2867026/diff/1/4#newcode144
net/base/x509_certificate.cc:144: // But we own the input OSCertHandle, which
makes it our job to free it.
Delete this comment, because the FreeOSCertHandle call it
refers to has been deleted.

http://codereview.chromium.org/2867026/diff/1/4#newcode175
net/base/x509_certificate.cc:175: : source_(source) {
It is fine to do
    : cert_handle_(DupOSCertHandle(cert_handle)),
in the constructor's initializer list.  A lot of Chromium
code does that.

I guess you want to put the DupOSCertHandle(cert_handle)
call right next to the related code that duplicates the
handles in |intermediates|?

http://codereview.chromium.org/2867026/diff/1/5
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/2867026/diff/1/5#newcode90
net/base/x509_certificate.h:90: // (http://crbug.com/7065).
Should we document that the function duplicates |cert_handle|
and the OS certificate handles in |intermediates|?  Or is that
the obvious behavior expected by most people (which is why
we're making this change)?

http://codereview.chromium.org/2867026/diff/1/6
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/2867026/diff/1/6#newcode1
net/base/x509_certificate_mac.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
You modified x509_certificate_mac.cc and x509_certificate_win.cc.

Just wanted to make sure x509_certificate_nss.cc does NOT
need to be modified.

http://codereview.chromium.org/2867026/diff/1/6#newcode794
net/base/x509_certificate_mac.cc:794: err = SecIdentityCopyCertificate(identity,
&cert_handle);
ISSUE: does this work?  We may need to do something like:
    SecCertificateRef cert_handle;
    err = SecIdentityCopyCertificate(identity, &cert_handle);
    if (err != noErr)
      continue;
    scoped_cftyperef<SecCertificateRef> scoped_cert_handle(cert_handle);

http://codereview.chromium.org/2867026/diff/1/7
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/2867026/diff/1/7#newcode375
net/base/x509_certificate_unittest.cc:375:
X509Certificate::FreeOSCertHandle(google_cert_handle);
I think it is time to add a X509Certificate::ScopedOSCertHandle
type...

http://codereview.chromium.org/2867026/diff/1/10
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/2867026/diff/1/10#newcode1262
net/socket/ssl_client_socket_nss.cc:1262:
net::X509Certificate::FreeOSCertHandle(cert_context2);
Remove net:: because this is in the net namespace.
You can also call CERTFreeCertificateContext here.

http://codereview.chromium.org/2867026/diff/1/10#newcode1347
net/socket/ssl_client_socket_nss.cc:1347: CERT_DestroyCertificate(cert);
I think we have a ScopedNSSCertificate type that we can
use...

http://codereview.chromium.org/2867026/diff/1/11
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/2867026/diff/1/11#newcode420
net/socket/ssl_client_socket_win.cc:420:
net::X509Certificate::FreeOSCertHandle(cert_context2);
Remove net::, or call CERTFreeCertificateContext.

See als line 1319 below.

[Ryan Sleevi, 2010-06-26 18:16:24]

Fixed the nits, save for introducing a X509Certificate::ScopedOSCertHandle.

http://codereview.chromium.org/2867026/diff/1/5
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/2867026/diff/1/5#newcode90
net/base/x509_certificate.h:90: // (http://crbug.com/7065).
On 2010/06/26 17:06:22, wtc wrote:
> Should we document that the function duplicates |cert_handle|
> and the OS certificate handles in |intermediates|?  Or is that
> the obvious behavior expected by most people (which is why
> we're making this change)?

My inclination is to say "obvious", but that's more likely because it's what I
expected rather than a self-evident property. A quick scan through other classes
in the net:: namespace, and I didn't see notice any headers talking about
copying - they only referenced ownership when they explicitly assumed or
released it. The only problem I'd expect would be from someone used to the "old"
style, but there were only a few places where that was ever done.

http://codereview.chromium.org/2867026/diff/1/6
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/2867026/diff/1/6#newcode1
net/base/x509_certificate_mac.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
On 2010/06/26 17:06:22, wtc wrote:
> You modified x509_certificate_mac.cc and x509_certificate_win.cc.
> 
> Just wanted to make sure x509_certificate_nss.cc does NOT
> need to be modified.

Correct. Both Mac and Win had helper functions to support them that ended up
dup'ing the contexts, whereas NSS interacted only with the common
implementation. Thus, fixing the common implementation was all NSS needed.

http://codereview.chromium.org/2867026/diff/1/7
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/2867026/diff/1/7#newcode375
net/base/x509_certificate_unittest.cc:375:
X509Certificate::FreeOSCertHandle(google_cert_handle);
Created a scoped handle as a member typedef of X509Certificate would, for NSS at
least, force either inclusion of cert.h or require a forward declaration of
CERT_DestroyCertificate. Windows/Mac would be unaffected, since they're already
pulling in system headers.

Given that X509Certificate.h is not alone in trying to avoid including NSS
headers, I wasn't sure if there is a specific reason, or it it's just the
general style guide (forward declare as much as possible) at work.

If the latter, I'll go ahead and add it, I just wanted to double check in the
event it was the former.

http://codereview.chromium.org/2867026/diff/1/10
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/2867026/diff/1/10#newcode1347
net/socket/ssl_client_socket_nss.cc:1347: CERT_DestroyCertificate(cert);
On 2010/06/26 17:06:22, wtc wrote:
> I think we have a ScopedNSSCertificate type that we can
> use...

It's within x509_certificate_nss.cc, in an anonymous namespace. I would have
promoted it, except for the previous comments on NSS headers in x509_certificate

[wtc, 2010-06-26 18:30:33]

LGTM!  Just one nit below.  You can check this in.  Thanks!

http://codereview.chromium.org/2867026/diff/1/4
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/2867026/diff/1/4#newcode143
net/base/x509_certificate.cc:143: // Return the certificate with the same
fingerprint from our cache.
Keep line 143.
Delete only line 144.