Address post-review feedback for r82214. In addition, no longer attempt to gracefully recover from read errors when deserializing a X509Certificate from a Pickle.

R=wtc
BUG=7065
TEST=none

Review URL: http://codereview.chromium.org/6880094

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@82618 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 40 insertions, 46 deletions

diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index b371c97..73636d7 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -236,40 +236,30 @@ X509Certificate* X509Certificate::CreateFromBytes(const char* data,
 X509Certificate* X509Certificate::CreateFromPickle(const Pickle& pickle,
                                                    void** pickle_iter,
                                                    PickleType type) {
-  OSCertHandle cert_handle = ReadCertHandleFromPickle(pickle, pickle_iter);
-  OSCertHandles intermediates;
+  OSCertHandle cert_handle = ReadOSCertHandleFromPickle(pickle, pickle_iter);
+  if (!cert_handle)
+    return NULL;
 
-  // Even if a certificate fails to parse, whether the server certificate in
-  // |cert_handle| or one of the optional intermediates, continue reading
-  // the data from |pickle| so that |pickle_iter| is kept in sync for any
-  // other reads the caller may perform after this method returns.
+  OSCertHandles intermediates;
+  size_t num_intermediates = 0;
   if (type == PICKLETYPE_CERTIFICATE_CHAIN) {
-    size_t num_intermediates;
     if (!pickle.ReadSize(pickle_iter, &num_intermediates)) {
       FreeOSCertHandle(cert_handle);
       return NULL;
     }
 
-    bool ok = !!cert_handle;
     for (size_t i = 0; i < num_intermediates; ++i) {
-      OSCertHandle intermediate = ReadCertHandleFromPickle(pickle,
-                                                           pickle_iter);
-      // If an intermediate fails to load, it and any certificates after it
-      // will not be added. However, any intermediates that were successfully
-      // parsed before the failure can be safely returned.
-      ok &= !!intermediate;
-      if (ok) {
-        intermediates.push_back(intermediate);
-      } else if (intermediate) {
-        FreeOSCertHandle(intermediate);
-      }
+      OSCertHandle intermediate = ReadOSCertHandleFromPickle(pickle,
+                                                             pickle_iter);
+      if (!intermediate)
+        break;
+      intermediates.push_back(intermediate);
     }
   }
 
-  if (!cert_handle)
-    return NULL;
-  X509Certificate* cert = CreateFromHandle(cert_handle, SOURCE_FROM_CACHE,
-                                           intermediates);
+  X509Certificate* cert = NULL;
+  if (intermediates.size() == num_intermediates)
+    cert = CreateFromHandle(cert_handle, SOURCE_FROM_CACHE, intermediates);
   FreeOSCertHandle(cert_handle);
   for (size_t i = 0; i < intermediates.size(); ++i)
     FreeOSCertHandle(intermediates[i]);
@@ -357,7 +347,7 @@ CertificateList X509Certificate::CreateCertificateListFromBytes(
 
 void X509Certificate::Persist(Pickle* pickle) {
   DCHECK(cert_handle_);
-  if (!WriteCertHandleToPickle(cert_handle_, pickle)) {
+  if (!WriteOSCertHandleToPickle(cert_handle_, pickle)) {
     NOTREACHED();
     return;
   }
@@ -368,7 +358,7 @@ void X509Certificate::Persist(Pickle* pickle) {
   }
 
   for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
-    if (!WriteCertHandleToPickle(intermediate_ca_certs_[i], pickle)) {
+    if (!WriteOSCertHandleToPickle(intermediate_ca_certs_[i], pickle)) {
       NOTREACHED();
       return;
     }
diff --git a/net/base/x509_certificate.h b/net/base/x509_certificate.h
index 6376b43..f6d3502 100644
--- a/net/base/x509_certificate.h
+++ b/net/base/x509_certificate.h
@@ -113,6 +113,10 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
                   FORMAT_PKCS7,
   };
 
+  // PickleType is intended for deserializing certificates that were pickled
+  // by previous releases as part of a net::HttpResponseInfo, which in version
+  // 1 only contained a single certificate. When serializing certificates to a
+  // new Pickle, PICKLETYPE_CERTIFICATE_CHAIN is always used.
   enum PickleType {
     // When reading a certificate from a Pickle, the Pickle only contains a
     // single certificate.
@@ -409,11 +413,11 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
   // not guaranteed to be the same across different underlying cryptographic
   // libraries, nor acceptable to CreateFromBytes(). Returns an invalid
   // handle, NULL, on failure.
-  static OSCertHandle ReadCertHandleFromPickle(const Pickle& pickle,
-                                               void** pickle_iter);
+  static OSCertHandle ReadOSCertHandleFromPickle(const Pickle& pickle,
+                                                 void** pickle_iter);
 
   // Writes a single certificate to |pickle|. Returns false on failure.
-  static bool WriteCertHandleToPickle(OSCertHandle handle, Pickle* pickle);
+  static bool WriteOSCertHandleToPickle(OSCertHandle handle, Pickle* pickle);
 
   // The subject of the certificate.
   CertPrincipal subject_;
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index f7dfc66..3b51777 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -1330,8 +1330,8 @@ CFArrayRef X509Certificate::CreateClientCertificateChain() const {
 
 // static
 X509Certificate::OSCertHandle
-X509Certificate::ReadCertHandleFromPickle(const Pickle& pickle,
-                                          void** pickle_iter) {
+X509Certificate::ReadOSCertHandleFromPickle(const Pickle& pickle,
+                                            void** pickle_iter) {
   const char* data;
   int length;
   if (!pickle.ReadData(pickle_iter, &data, &length))
@@ -1341,8 +1341,8 @@ X509Certificate::ReadCertHandleFromPickle(const Pickle& pickle,
 }
 
 // static
-bool X509Certificate::WriteCertHandleToPickle(OSCertHandle cert_handle,
-                                              Pickle* pickle) {
+bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
+                                                Pickle* pickle) {
   CSSM_DATA cert_data;
   OSStatus status = SecCertificateGetData(cert_handle, &cert_data);
   if (status)
diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index 14004e0..a6d0663 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -991,8 +991,8 @@ SHA1Fingerprint X509Certificate::CalculateFingerprint(
 
 // static
 X509Certificate::OSCertHandle
-X509Certificate::ReadCertHandleFromPickle(const Pickle& pickle,
-                                          void** pickle_iter) {
+X509Certificate::ReadOSCertHandleFromPickle(const Pickle& pickle,
+                                            void** pickle_iter) {
   const char* data;
   int length;
   if (!pickle.ReadData(pickle_iter, &data, &length))
@@ -1002,8 +1002,8 @@ X509Certificate::ReadCertHandleFromPickle(const Pickle& pickle,
 }
 
 // static
-bool X509Certificate::WriteCertHandleToPickle(OSCertHandle cert_handle,
-                                              Pickle* pickle) {
+bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
+                                                Pickle* pickle) {
   return pickle->WriteData(
       reinterpret_cast<const char*>(cert_handle->derCert.data),
       cert_handle->derCert.len);
diff --git a/net/base/x509_certificate_openssl.cc b/net/base/x509_certificate_openssl.cc
index 10ce266..bbb5782 100644
--- a/net/base/x509_certificate_openssl.cc
+++ b/net/base/x509_certificate_openssl.cc
@@ -515,8 +515,8 @@ bool X509Certificate::IsSameOSCert(X509Certificate::OSCertHandle a,
 
 // static
 X509Certificate::OSCertHandle
-X509Certificate::ReadCertHandleFromPickle(const Pickle& pickle,
-                                          void** pickle_iter) {
+X509Certificate::ReadOSCertHandleFromPickle(const Pickle& pickle,
+                                            void** pickle_iter) {
   const char* data;
   int length;
   if (!pickle.ReadData(pickle_iter, &data, &length))
@@ -526,8 +526,8 @@ X509Certificate::ReadCertHandleFromPickle(const Pickle& pickle,
 }
 
 // static
-bool X509Certificate::WriteCertHandleToPickle(OSCertHandle cert_handle,
-                                              Pickle* pickle) {
+bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
+                                                Pickle* pickle) {
   DERCache der_cache;
   if (!GetDERAndCacheIfNeeded(cert_handle, &der_cache))
     return false;
diff --git a/net/base/x509_certificate_win.cc b/net/base/x509_certificate_win.cc
index fd5076d..367f4ed 100644
--- a/net/base/x509_certificate_win.cc
+++ b/net/base/x509_certificate_win.cc
@@ -1006,8 +1006,8 @@ SHA1Fingerprint X509Certificate::CalculateFingerprint(
 
 // static
 X509Certificate::OSCertHandle
-X509Certificate::ReadCertHandleFromPickle(const Pickle& pickle,
-                                          void** pickle_iter) {
+X509Certificate::ReadOSCertHandleFromPickle(const Pickle& pickle,
+                                            void** pickle_iter) {
   const char* data;
   int length;
   if (!pickle.ReadData(pickle_iter, &data, &length))
@@ -1026,8 +1026,8 @@ X509Certificate::ReadCertHandleFromPickle(const Pickle& pickle,
 }
 
 // static
-bool X509Certificate::WriteCertHandleToPickle(OSCertHandle cert_handle,
-                                              Pickle* pickle) {
+bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
+                                                Pickle* pickle) {
   DWORD length = 0;
   if (!CertSerializeCertificateStoreElement(cert_handle, 0, NULL, &length))
     return false;
diff --git a/net/http/http_response_info.cc b/net/http/http_response_info.cc
index 49090e0..5aee4b1 100644
--- a/net/http/http_response_info.cc
+++ b/net/http/http_response_info.cc
@@ -31,6 +31,8 @@ enum {
   RESPONSE_INFO_VERSION_MASK = 0xFF,
 
   // This bit is set if the response info has a cert at the end.
+  // Version 1 serialized only the end-entity certificate, while subsequent
+  // versions include the available certificate chain.
   RESPONSE_INFO_HAS_CERT = 1 << 8,
 
   // This bit is set if the response info has a security-bits field (security
@@ -135,8 +137,6 @@ bool HttpResponseInfo::InitFromPickle(const Pickle& pickle,
 
   // read ssl-info
   if (flags & RESPONSE_INFO_HAS_CERT) {
-    // Version 1 only serialized only the end-entity certificate,
-    // while subsequent versions include the entire chain.
     X509Certificate::PickleType type = (version == 1) ?
         X509Certificate::PICKLETYPE_SINGLE_CERTIFICATE :
         X509Certificate::PICKLETYPE_CERTIFICATE_CHAIN;