Treat HSTS and HPKP state independently.

net/http/http_security_headers.cc

 // Copyright (c) 2012 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.
 
 #include "base/base64.h"
 #include "base/basictypes.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/string_util.h"
 #include "net/http/http_security_headers.h"
@@ skipping 267 matching lines @@
                      base::TimeDelta* max_age,
                      bool* include_subdomains,
                      HashValueVector* hashes) {
   bool parsed_max_age = false;
   bool include_subdomains_candidate = false;
   uint32 max_age_candidate = 0;
   HashValueVector pins;
 
   std::string source = value;
 
+  hashes->clear();

[Ryan Sleevi, 2015/01/13 21:56:40]

BUG: You should not clear this unless things parse successfully. Doesn't this
have the side-effect of nuking the effective pins if a header fails to parse?
Either way, it's "surprising" (note how we intentionally don't update return
values until line 325, when we're guaranteed success)

[davidben, 2015/01/13 23:30:47]

Nah. AddHPKPHeader only ever calls it on a temporary now. (What I meant when I
said I accidentally fixed the pins overlaying.) Though I agree that shouldn't
nuke it. Done.

   while (!source.empty()) {
     StringPair semicolon = Split(source, ';');
     semicolon.first = Strip(semicolon.first);
     semicolon.second = Strip(semicolon.second);
     StringPair equals = Split(semicolon.first, '=');
     equals.first = Strip(equals.first);
     equals.second = Strip(equals.second);
 
     if (LowerCaseEqualsASCII(equals.first, "max-age")) {
       if (equals.second.empty() ||
@@ skipping 20 matching lines @@
   if (!parsed_max_age)
     return false;
 
   if (!IsPinListValid(pins, chain_hashes))
     return false;
 
   *max_age = base::TimeDelta::FromSeconds(max_age_candidate);
   *include_subdomains = include_subdomains_candidate;
   for (HashValueVector::const_iterator i = pins.begin();
        i != pins.end(); ++i) {
-    bool found = false;
-
-    for (HashValueVector::const_iterator j = hashes->begin();
-         j != hashes->end(); ++j) {
-      if (j->Equals(*i)) {
-        found = true;
-        break;
-      }
-    }
-
-    if (!found)
-      hashes->push_back(*i);
+    hashes->push_back(*i);

[Ryan Sleevi, 2015/01/13 21:56:40]

These are both HashValueVectors.

Why don't you just hashes->swap(pins)?

[davidben, 2015/01/13 23:30:47]

Yeah, that's better. Done.

   }
 
   return true;
 }
 
 }  // namespace net