Ensure X509Certificate::OSCertHandles are safe to be used on both UI and IO threads on Win

net/base/x509_certificate.h

 // Copyright (c) 2011 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.
 
 #ifndef NET_BASE_X509_CERTIFICATE_H_
 #define NET_BASE_X509_CERTIFICATE_H_
 #pragma once
 
 #include <string.h>
 
 #include <string>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/string_piece.h"
 #include "base/time.h"
 #include "net/base/net_api.h"
 #include "net/base/x509_cert_types.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <wincrypt.h>
 #elif defined(OS_MACOSX)
 #include <CoreFoundation/CFArray.h>
 #include <Security/SecBase.h>
 
 #include "base/synchronization/lock.h"
 #elif defined(USE_OPENSSL)
+#include <openssl/safestack.h>
 // Forward declaration; real one in <x509.h>
-struct x509_st;
+typedef struct x509_st X509;
+PREDECLARE_STACK_OF(X509);
 typedef struct x509_store_st X509_STORE;
 #elif defined(USE_NSS)
 // Forward declaration; real one in <cert.h>
 struct CERTCertificateStr;
 #endif
 
 class Pickle;
 
 namespace crypto {
 class StringPiece;
 class RSAPrivateKey;
 }  // namespace crypto
 
 namespace net {
 
 class CertVerifyResult;
 
 typedef std::vector<scoped_refptr<X509Certificate> > CertificateList;
 
 // X509Certificate represents a X.509 certificate, which is comprised a
 // particular identity or end-entity certificate, such as an SSL server
 // identity or an SSL client certificate, and zero or more intermediate
 // certificates that may be used to build a path to a root certificate.
 class NET_API X509Certificate
     : public base::RefCountedThreadSafe<X509Certificate> {
  public:
-  // A handle to the certificate object in the underlying crypto library.
-  // We assume that OSCertHandle is a pointer type on all platforms and
-  // NULL is an invalid OSCertHandle.
+  // An OSCertHandle is a handle to the certificate object in the underlying
+  // crypto library. We assume that OSCertHandle is a pointer type on all
+  // platforms and that NULL represents an invalid OSCertHandle.
+  //
+  // An OSCertListHandle is a handle to the underlying crypto library that

[wtc, 2011/10/04 00:26:34]

Add "the object in" before "the underlying crypto library".

+  // represents a collection of certificates, with one of the certificates
+  // marked as an identity certificate and the remaining certificates marked

[wtc, 2011/10/04 00:26:34]

I suggest changing "an identity certificate" to
"a primary certificate" (or "the primary certificate"?).
The meaning of "identity certificate" is not clear here.

[Ryan Sleevi, 2011/10/04 03:38:07]

Does the explanation on line 53-54 provide the necessary context?

[wtc, 2011/10/04 18:00:51]

This is fine.  My complaint about this comment is that
every certificate we deal with here is an identity certificate,
so we need a way to distinguish the cert represented by
os_cert_handle_ and the other certs used for chain building.
This is why I suggested "primary" vs. "supplementary".

+  // as supplementary certificates for path building. Like OSCertHandle, it
+  // is assumed to be a pointer type on all platforms and that NULL
+  // represents an invalid OSCertListHandle.
+  //
+  // It should be noted that depending on the underlying cryptographic

[wtc, 2011/10/04 00:26:34]

Nit: for brevity, change "It should be noted that" to
"Note: " or "Note that", or just remove it.

+  // library, an OSCertHandle or OSCertListHandle may not be thread-safe.

[wtc, 2011/10/04 00:26:34]

Please add a comment to motivate OSCertListHandle.  I think
it is for thread safety.  Since OSCertListHandle may not be
thread-safe, we create an OSCertListHandle for each thread
that needs one.  Do I understand this correctly?

 #if defined(OS_WIN)
   typedef PCCERT_CONTEXT OSCertHandle;
+  // Though the same type as an OSCertHandle, a unique HCERTSTORE member is
+  // used for the certificate containing just the subset of related

[wtc, 2011/10/04 00:26:34]

Add "store" after "certificate".

Change "related" to "supplementary".

+  // certificates.
+  typedef PCCERT_CONTEXT OSCertListHandle;
 #elif defined(OS_MACOSX)
   typedef SecCertificateRef OSCertHandle;
+  typedef CFArrayRef OSCertListHandle;
 #elif defined(USE_OPENSSL)
   typedef struct x509_st* OSCertHandle;

[wtc, 2011/10/04 00:26:34]

Change "struct x509_st" to X509.

+  typedef STACK_OF(X509)* OSCertListHandle;
 #elif defined(USE_NSS)
   typedef struct CERTCertificateStr* OSCertHandle;
+  // TODO(rsleevi): With NSS, it is not currently necessary to use a
+  // separate type, because of how certificate path building/verification is
+  // implemented.
+  typedef OSCertHandle OSCertListHandle;
 #else
   // TODO(ericroman): not implemented
   typedef void* OSCertHandle;
+  typedef void* OSCertListHandle;

[wtc, 2011/10/04 00:26:34]

Nit: it would be nice to use the same style of typedef
when OSCertListHandle is the same as OSCertHandle.

On line 78 and line 94, we duplicate the typedef.
On line 90, we define OSCertListHandle as OSCertHandle.

 #endif
 
   typedef std::vector<OSCertHandle> OSCertHandles;
 
   // Predicate functor used in maps when X509Certificate is used as the key.
   class NET_API LessThan {
    public:
     bool operator() (X509Certificate* lhs,  X509Certificate* rhs) const;
   };
 
@@ skipping 146 matching lines @@
   const OSCertHandles& GetIntermediateCertificates() const {
     return intermediate_ca_certs_;
   }
 
   // Returns true if I already contain the given intermediate cert.
   bool HasIntermediateCertificate(OSCertHandle cert);
 
   // Returns true if I already contain all the given intermediate certs.
   bool HasIntermediateCertificates(const OSCertHandles& certs);
 
+  // Returns a new OSCertListHandle representing the certificate and any
+  // associated intermediates, or NULL on failure. Ownership is transferred

[wtc, 2011/10/04 00:26:34]

Nit: intermediates => intermediate certificates
                 or   intermediate CA certificates

+  // to the caller and may be released by calling FreeOSCertListHandle()
+  // with the returned value.
+  OSCertListHandle CreateOSCertListHandle() const;
+
 #if defined(OS_MACOSX)
   // Does this certificate's usage allow SSL client authentication?
   bool SupportsSSLClientAuth() const;
 
   // Do any of the given issuer names appear in this cert's chain of trust?
   bool IsIssuedBy(const std::vector<CertPrincipal>& valid_issuers);
 
   // Creates a security policy for certificates used as client certificates
   // in SSL.
   // If a policy is successfully created, it will be stored in
@@ skipping 90 matching lines @@
   // specific |format|. Returns an empty collection on failure.
   static OSCertHandles CreateOSCertHandlesFromBytes(
       const char* data, int length, Format format);
 
   // Duplicates (or adds a reference to) an OS certificate handle.
   static OSCertHandle DupOSCertHandle(OSCertHandle cert_handle);
 
   // Frees (or releases a reference to) an OS certificate handle.
   static void FreeOSCertHandle(OSCertHandle cert_handle);
 
+  // Frees (or releases a reference to) an OS certificate list handle.
+  static void FreeOSCertListHandle(OSCertListHandle cert_list);

[wtc, 2011/10/04 00:26:34]

cert_list => cert_list_handle

+
   // Calculates the SHA-1 fingerprint of the certificate.  Returns an empty
   // (all zero) fingerprint on failure.
   static SHA1Fingerprint CalculateFingerprint(OSCertHandle cert_handle);
 
  private:
   friend class base::RefCountedThreadSafe<X509Certificate>;
   friend class TestRootCerts;  // For unit tests
   FRIEND_TEST_ALL_PREFIXES(X509CertificateTest, Cache);
   FRIEND_TEST_ALL_PREFIXES(X509CertificateTest, IntermediateCertificates);
   FRIEND_TEST_ALL_PREFIXES(X509CertificateTest, SerialNumbers);
@@ skipping 102 matching lines @@
   // (Marked mutable because it's used in a const method.)
   mutable base::Lock verification_lock_;
 #endif
 
   DISALLOW_COPY_AND_ASSIGN(X509Certificate);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_X509_CERTIFICATE_H_