Reduce memory usage of CRLSets

Now that CRLSets no longer use a bespoke delta update
mechanism, reduce the amount of internal state that needs
to be tracked by the CRLSet. This avoids a duplicating the
issuer key hashes.

Bug: None
Change-Id: Id6d1ba752972b63ac4109a91d2a9e17794cf3b1e
Reviewed-on: https://chromium-review.googlesource.com/1132105
Commit-Queue: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574069}

net/cert/crl_set.cc

@@ -141,15 +141,12 @@ bool CopyHashListFromHeader(base::DictionaryValue* header_dict,
 
 // CopyHashToHashesMapFromHeader parse a map from base64-encoded, SHA-256
 // hashes to lists of the same, from the given |key| in |header_dict|. It
-// copies the map data into |out| (after base64-decoding) and writes the order
-// of map keys into |out_key_order|.
+// copies the map data into |out| (after base64-decoding).
 bool CopyHashToHashesMapFromHeader(
     base::DictionaryValue* header_dict,
     const char* key,
-    std::unordered_map<std::string, std::vector<std::string>>* out,
-    std::vector<std::string>* out_key_order) {
+    std::unordered_map<std::string, std::vector<std::string>>* out) {
   out->clear();
-  out_key_order->clear();
 
   base::Value* const dict =
       header_dict->FindKeyOfType(key, base::Value::Type::DICTIONARY);
@@ -177,7 +174,6 @@ bool CopyHashToHashesMapFromHeader(
       return false;
     }
 
-    out_key_order->push_back(subject_hash);
     (*out)[subject_hash] = allowed_spkis;
   }
 
@@ -240,27 +236,19 @@ bool CRLSet::Parse(base::StringPiece data, scoped_refptr<CRLSet>* out_crl_set) {
   crl_set->crls_.reserve(64);  // Value observed experimentally.
 
   for (size_t crl_index = 0; !data.empty(); crl_index++) {
-    // Speculatively push back a pair and pass it to ReadCRL() to avoid
-    // unnecessary copies.
-    crl_set->crls_.push_back(
-        std::make_pair(std::string(), std::vector<std::string>()));
-    std::pair<std::string, std::vector<std::string>>* const back_pair =
-        &crl_set->crls_.back();
-
-    if (!ReadCRL(&data, &back_pair->first, &back_pair->second)) {
-      // Undo the speculative push_back() performed above.
-      crl_set->crls_.pop_back();
+    std::string spki_hash;
+    std::vector<std::string> blocked_serials;
+
+    if (!ReadCRL(&data, &spki_hash, &blocked_serials)) {
       return false;
     }
-
-    crl_set->crls_index_by_issuer_[back_pair->first] = crl_index;
+    crl_set->crls_[std::move(spki_hash)] = std::move(blocked_serials);
   }
 
   if (!CopyHashListFromHeader(header_dict.get(), "BlockedSPKIs",
                               &crl_set->blocked_spkis_) ||
       !CopyHashToHashesMapFromHeader(header_dict.get(), "LimitedSubjects",
-                                     &crl_set->limited_subjects_,
-                                     &crl_set->limited_subjects_ordered_)) {
+                                     &crl_set->limited_subjects_)) {
     return false;
   }
 
@@ -312,15 +300,12 @@ CRLSet::Result CRLSet::CheckSerial(
   while (serial.size() > 1 && serial[0] == 0x00)
     serial.remove_prefix(1);
 
-  std::unordered_map<std::string, size_t>::const_iterator crl_index =
-      crls_index_by_issuer_.find(issuer_spki_hash.as_string());
-  if (crl_index == crls_index_by_issuer_.end())
+  auto it = crls_.find(issuer_spki_hash.as_string());
+  if (it == crls_.end())
     return UNKNOWN;
-  const std::vector<std::string>& serials = crls_[crl_index->second].second;
 
-  for (std::vector<std::string>::const_iterator i = serials.begin();
-       i != serials.end(); ++i) {
-    if (base::StringPiece(*i) == serial)
+  for (const auto& issuer_serial : it->second) {
+    if (issuer_serial == serial)
       return REVOKED;
   }
 
@@ -339,7 +324,7 @@ uint32_t CRLSet::sequence() const {
   return sequence_;
 }
 
-const CRLSet::CRLList& CRLSet::crls() const {
+const CRLSet::CRLList& CRLSet::CrlsForTesting() const {
   return crls_;
 }
 
@@ -392,15 +377,15 @@ scoped_refptr<CRLSet> CRLSet::ForTesting(
   if (is_expired)
     crl_set->not_after_ = 1;
 
-  if (issuer_spki != nullptr) {
+  if (issuer_spki) {
     const std::string spki(reinterpret_cast<const char*>(issuer_spki->data),
                            sizeof(issuer_spki->data));
-    crl_set->crls_.push_back(make_pair(spki, std::vector<std::string>()));
-    crl_set->crls_index_by_issuer_[spki] = 0;
-  }
+    std::vector<std::string> serials;
+    if (!serial_number.empty())
+      serials.push_back(serial_number);
 
-  if (!serial_number.empty())
-    crl_set->crls_[0].second.push_back(serial_number);
+    crl_set->crls_.emplace(std::move(spki), std::move(serials));
+  }
 
   if (!subject_hash.empty())
     crl_set->limited_subjects_[subject_hash] = acceptable_spki_hashes_for_cn;

net/cert/crl_set.h

@@ -64,14 +64,13 @@ class NET_EXPORT CRLSet : public base::RefCountedThreadSafe<CRLSet> {
   // numbers.
   uint32_t sequence() const;
 
-  // CRLList contains a list of (issuer SPKI hash, revoked serial numbers)
+  // CRLList contains a map of (issuer SPKI hash, revoked serial numbers)
   // pairs.
-  typedef std::vector< std::pair<std::string, std::vector<std::string> > >
-      CRLList;
+  typedef std::unordered_map<std::string, std::vector<std::string>> CRLList;
 
   // crls returns the internal state of this CRLSet. It should only be used in
   // testing.
-  const CRLList& crls() const;
+  const CRLList& CrlsForTesting() const;
 
   // EmptyCRLSetForTesting returns a valid, but empty, CRLSet for unit tests.
   static scoped_refptr<CRLSet> EmptyCRLSetForTesting();
@@ -101,25 +100,18 @@ class NET_EXPORT CRLSet : public base::RefCountedThreadSafe<CRLSet> {
   friend class base::RefCountedThreadSafe<CRLSet>;
 
   uint32_t sequence_;
-  CRLList crls_;
   // not_after_ contains the time, in UNIX epoch seconds, after which the
   // CRLSet should be considered stale, or 0 if no such time was given.
   uint64_t not_after_;
-  // crls_index_by_issuer_ maps from issuer SPKI hashes to the index in |crls_|
-  // where the information for that issuer can be found. We have both |crls_|
-  // and |crls_index_by_issuer_| because, when applying a delta update, we need
-  // to identify a CRL by index.
-  std::unordered_map<std::string, size_t> crls_index_by_issuer_;
+  // crls_ is a map from the SHA-256 hash of an X.501 subject name to a list
+  // of revoked serial numbers.
+  CRLList crls_;
   // blocked_spkis_ contains the SHA256 hashes of SPKIs which are to be blocked
   // no matter where in a certificate chain they might appear.
   std::vector<std::string> blocked_spkis_;
   // limited_subjects_ is a map from the SHA256 hash of an X.501 subject name
   // to a list of allowed SPKI hashes for certificates with that subject name.
   std::unordered_map<std::string, std::vector<std::string>> limited_subjects_;
-  // limited_subjects_ordered_ contains the keys of |limited_subjects_|,
-  // ordered in the same order as they were found when parsing. This allows
-  // exact reserialisation.
-  std::vector<std::string> limited_subjects_ordered_;
 };
 
 }  // namespace net

net/cert/crl_set_unittest.cc

@@ -84,9 +84,9 @@ TEST(CRLSetTest, Parse) {
   EXPECT_TRUE(CRLSet::Parse(s, &set));
   ASSERT_TRUE(set.get() != NULL);
 
-  const CRLSet::CRLList& crls = set->crls();
+  const CRLSet::CRLList& crls = set->CrlsForTesting();
   ASSERT_EQ(1u, crls.size());
-  const std::vector<std::string>& serials = crls[0].second;
+  const std::vector<std::string>& serials = crls.begin()->second;
   static const unsigned kExpectedNumSerials = 13;
   ASSERT_EQ(kExpectedNumSerials, serials.size());
   EXPECT_EQ(std::string("\x10\x0D\x7F\x30\x00\x03\x00\x00\x23\xB0", 10),