Improve error handling of the transport security state generator.

net/tools/transport_security_state_generator/transport_security_state_generator.cc

Index: net/tools/transport_security_state_generator/transport_security_state_generator.cc
diff --git a/net/tools/transport_security_state_generator/transport_security_state_generator.cc b/net/tools/transport_security_state_generator/transport_security_state_generator.cc
index 8473414876fc2d9cb5e0af18dbc0d26f07ea49cd..321cc2055f47db351782401aefb468bc5fb9d662 100644
--- a/net/tools/transport_security_state_generator/transport_security_state_generator.cc
+++ b/net/tools/transport_security_state_generator/transport_security_state_generator.cc
@@ -37,12 +37,29 @@ using net::transport_security_state::SPKIHash;
 
 namespace {
 
+using ErrorList = std::vector<std::string>;

[Ryan Sleevi, 2017/02/14 20:53:17]

Why error list instead of VLOG?

[martijnc, 2017/02/15 22:07:59]

With this I can print the errors directly to the console. I'm not very familiar
with VLOG but the documentation indicates you need to pass additional parameters
(and on Windows you need Sawbuck) the get VLOG output.

I think it is easier here to just print the errors immediately. Having the tool
fail during the build (or when it is executed manually) without error output
might be confusing?

[Ryan Sleevi, 2017/02/16 01:50:40]

Windows wouldn't require Sawbuck here - VLOG will dump to stderr, but yes, it
requires you specify the vlog level (--v=1 / --v=2, etc)
So that's a really good argument about "Why not VLOG" - since this is part of
the build process and clearly communicating errors is good - but I guess I'm
then confused why the ErrorList rather than splitting to std::cerr directly,
since this is a command-line tool generating things.

Or using the more general pattern, why not just LOG(ERROR) << stuff and use that
pattern?

I'm trying to understand the design decision/rationale a bit more, since what
you have is technically valid, but it seems to add a degree of complexity and
diversion from common patterns that I just wanted to make sure I understood, and
to see if there were any simpler solutions (or if they'd been ruled out)

[martijnc, 2017/02/16 18:02:46]

I want to avoid having multiple places/files that write output directly. My
thinking was to only write output from the main cc file
(transport_security_state_generator.cc). That puts this file in charge of all
output and ensures other files don't generate noise.

Although the functions that use ErrorList are currently in the main cc file,
they'll have to move to a separate file so that they can be tested as I've done
in [1].

That was my reasoning for using ErrorList but I'm happy to revert this or use
VLOG(ERROR) if that puts it in line with the general Chromium style.

[1] https://codereview.chromium.org/2660793002

+
 // Print the command line help.
 void PrintHelp() {
   std::cout << "transport_security_state_generator <json-file> <pins-file>"
             << " <template-file> <output-file> [-v]" << std::endl;
 }
 
+// Writes all error messages in |errors| to the output stream for errors.
+void PrintErrors(const ErrorList& errors) {
+  for (const auto& error : errors) {
+    std::cerr << error << std::endl;
+  }
+}
+
+// Converts a number to a string.
+// TODO(Martijnc): Remove once std::to_string is allowed.
+std::string ToString(size_t number) {
+  std::stringstream s;
+  s << number;
+  return s.str();
+}

[Ryan Sleevi, 2017/02/14 20:53:17]

This definitely feels like a cheat around the style guide, both around Streams (
https://google.github.io/styleguide/cppguide.html#Streams ) and to_string()
being forbidden.

Luckily, there's a non-cheaty way :)

https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h...

[martijnc, 2017/02/15 22:07:59]

Thanks, fixed.

+
 // Parses the |json| string and copies the items under the "entries" key to
 // |entries|, the pinsets under the "pinsets" key to |pinsets|, and the domain
 // IDs under the "domain_ids" key to |domain_ids|.
@@ -52,33 +69,37 @@ void PrintHelp() {
 bool ParseJSON(const std::string& json,
                TransportSecurityStateEntries* entries,
                Pinsets* pinsets,
-               DomainIDList* domain_ids) {
+               DomainIDList* domain_ids,
+               ErrorList* errors) {
+  bool success = true;
   std::unique_ptr<base::Value> value = base::JSONReader::Read(json);
   base::DictionaryValue* dict_value = nullptr;
   if (!value.get() || !value->GetAsDictionary(&dict_value)) {
-    std::cerr << "Could not parse the input JSON" << std::endl;
+    errors->push_back("Could not parse the input JSON file");
     return false;
   }
 
   const base::ListValue* preload_entries = nullptr;
   if (!dict_value->GetList("entries", &preload_entries)) {
-    std::cerr << "Could not parse the entries in the input JSON" << std::endl;
+    errors->push_back("Could not parse the entries in the input JSON");
     return false;
   }
 
   for (size_t i = 0; i < preload_entries->GetSize(); ++i) {
     const base::DictionaryValue* parsed = nullptr;
     if (!preload_entries->GetDictionary(i, &parsed)) {
-      std::cerr << "Could not parse entry " << i << std::endl;
-      return false;
+      errors->push_back("Could not parse entry " + ToString(i) +
+                        " in the input JSON");
+      success = false;

[Ryan Sleevi, 2017/02/14 20:53:17]

Why do you keep processing when success is false?

[martijnc, 2017/02/15 22:07:59]

I wanted to print a list of all non-fatal errors rather than just the first but
it might not be worth the additional code complexity.

Removed this logic.

     }
 
     std::unique_ptr<TransportSecurityStateEntry> entry(
         new TransportSecurityStateEntry());
 
     if (!parsed->GetString("name", &entry->hostname)) {
-      std::cerr << "Could not extract the name for entry " << i << std::endl;
-      return false;
+      errors->push_back("Could not extract the hostname for entry " +
+                        ToString(i) + " from the input JSON");
+      success = false;

[Ryan Sleevi, 2017/02/14 20:53:17]

Why do you keep processing when success is false?

[martijnc, 2017/02/15 22:07:59]

Removed (as above).

     }
 
     parsed->GetBoolean("include_subdomains", &entry->include_subdomains);
@@ -101,21 +122,23 @@ bool ParseJSON(const std::string& json,
 
   const base::ListValue* pinsets_list = nullptr;
   if (!dict_value->GetList("pinsets", &pinsets_list)) {
-    std::cerr << "Could not parse the pinsets in the input JSON" << std::endl;
+    errors->push_back("Could not parse the pinsets in the input JSON");
     return false;
   }
 
   for (size_t i = 0; i < pinsets_list->GetSize(); ++i) {
     const base::DictionaryValue* parsed = nullptr;
     if (!pinsets_list->GetDictionary(i, &parsed)) {
-      std::cerr << "Could not parse pinset " << i << std::endl;
-      return false;
+      errors->push_back("Could not parse pinset " + ToString(i) +
+                        " in the input JSON");
+      success = false;
     }
 
     std::string name;
     if (!parsed->GetString("name", &name)) {
-      std::cerr << "Could not extract the name for pinset " << i << std::endl;
-      return false;
+      errors->push_back("Could not extract the name for pinset " + ToString(i) +
+                        " from the input JSON");
+      success = false;
     }
 
     std::string report_uri;
@@ -149,7 +172,7 @@ bool ParseJSON(const std::string& json,
   // https://crbug.com/661206.
   const base::ListValue* domain_ids_list = nullptr;
   if (!dict_value->GetList("domain_ids", &domain_ids_list)) {
-    std::cerr << "Failed parsing JSON (domain_ids)" << std::endl;
+    errors->push_back("Could not parse the domain IDs in the input JSON");
     return false;
   }
 
@@ -159,7 +182,7 @@ bool ParseJSON(const std::string& json,
     domain_ids->push_back(domain);
   }
 
-  return true;
+  return success;
 }
 
 bool IsImportantWordInCertificateName(base::StringPiece name) {
@@ -189,11 +212,14 @@ std::string FilterName(base::StringPiece name) {
 
 // Returns true if |pin_name| is a reasonable match for the certificate name
 // |name|.
-bool MatchCertificateName(base::StringPiece name, base::StringPiece pin_name) {
+bool MatchCertificateName(base::StringPiece name,
+                          base::StringPiece pin_name,
+                          ErrorList* errors) {
   std::vector<base::StringPiece> words = base::SplitStringPiece(
       name, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
   if (words.empty()) {
-    std::cerr << "no words in certificate name" << std::endl;
+    errors->push_back("No words in certificate name for pin (" +
+                      pin_name.as_string() + ")");
     return false;
   }
   base::StringPiece first_word = words[0];
@@ -217,7 +243,8 @@ bool MatchCertificateName(base::StringPiece name, base::StringPiece pin_name) {
   }
 
   if (first_word.empty()) {
-    std::cerr << "first word of certificate name is empty" << std::endl;
+    errors->push_back("First word of certificate name (" + name.as_string() +
+                      ") is empty");
     return false;
   }
 
@@ -225,10 +252,9 @@ bool MatchCertificateName(base::StringPiece name, base::StringPiece pin_name) {
   first_word = filtered_word;
   if (!base::EqualsCaseInsensitiveASCII(pin_name.substr(0, first_word.size()),
                                         first_word)) {
-    std::cerr << "the first word of the certificate name ("
-              << first_word.as_string()
-              << ") isn't a prefix of the variable name ("
-              << pin_name.as_string() << ")" << std::endl;
+    errors->push_back(
+        "The first word of the certificate name (" + first_word.as_string() +
+        ") isn't a prefix of the variable name (" + pin_name.as_string() + ")");
     return false;
   }
 
@@ -240,21 +266,26 @@ bool MatchCertificateName(base::StringPiece name, base::StringPiece pin_name) {
 
       size_t pos = pin_name.find(class_name);
       if (pos == std::string::npos) {
-        std::cerr << "class specification doesn't appear in the variable name"
-                  << std::endl;
+        errors->push_back(
+            "Certficate class specification doesn't appear in the variable "
+            "name (" +
+            pin_name.as_string() + ")");
         return false;
       }
     } else if (word.size() == 1 && word[0] >= '0' && word[0] <= '9') {
       size_t pos = pin_name.find(word);
       if (pos == std::string::npos) {
-        std::cerr << "number doesn't appear in the variable name" << std::endl;
+        errors->push_back(
+            "Number doesn't appear in the certificate variable name (" +
+            pin_name.as_string() + ")");
         return false;
       }
     } else if (IsImportantWordInCertificateName(word)) {
       size_t pos = pin_name.find(word);
       if (pos == std::string::npos) {
-        std::cerr << word.as_string() << " doesn't appear in the variable name"
-                  << std::endl;
+        errors->push_back(word.as_string() +
+                          " doesn't appear in the certificate variable name (" +
+                          pin_name.as_string() + ")");
         return false;
       }
     }
@@ -300,7 +331,9 @@ enum class CertificateParserState {
 //
 // More info on the format can be found in
 // net/http/transport_security_state_static.pins
-bool ParseCertificatesFile(const std::string& certs_input, Pinsets* pinsets) {
+bool ParseCertificatesFile(const std::string& certs_input,
+                           Pinsets* pinsets,
+                           ErrorList* errors) {
   std::istringstream input_stream(certs_input);
   std::string line;
   CertificateParserState current_state = CertificateParserState::PRE_NAME;
@@ -324,7 +357,7 @@ bool ParseCertificatesFile(const std::string& certs_input, Pinsets* pinsets) {
     switch (current_state) {
       case CertificateParserState::PRE_NAME:
         if (!IsValidName(line)) {
-          std::cerr << "Invalid name in certificates file: " << line;
+          errors->push_back("Invalid name in pins file: " + line);
           return false;
         }
         name = line;
@@ -333,8 +366,7 @@ bool ParseCertificatesFile(const std::string& certs_input, Pinsets* pinsets) {
       case CertificateParserState::POST_NAME:
         if (base::StartsWith(line, kStartOfSHA256, compare_mode)) {
           if (!hash.FromString(line)) {
-            std::cerr << "Invalid hash value in certificate file for " << name
-                      << std::endl;
+            errors->push_back("Invalid hash value in pins file for " + name);
             return false;
           }
 
@@ -347,8 +379,7 @@ bool ParseCertificatesFile(const std::string& certs_input, Pinsets* pinsets) {
           buffer = line + '\n';
           current_state = CertificateParserState::IN_PUBLIC_KEY;
         } else {
-          std::cerr << "Invalid value in certificates file for " << name
-                    << std::endl;
+          errors->push_back("Invalid value in pins file for " + name);
           return false;
         }
         break;
@@ -360,26 +391,24 @@ bool ParseCertificatesFile(const std::string& certs_input, Pinsets* pinsets) {
 
         certificate = GetX509CertificateFromPEM(buffer);
         if (!certificate) {
-          std::cerr << "Could not parse certificate " << name << std::endl;
+          errors->push_back("Could not parse certificate " + name);
           return false;
         }
 
         if (!CalculateSPKIHashFromCertificate(certificate.get(), &hash)) {
-          std::cerr << "Could not extract SPKI from certificate " << name
-                    << std::endl;
+          errors->push_back("Could not extract SPKI from certificate " + name);
           return false;
         }
 
         if (!ExtractSubjectNameFromCertificate(certificate.get(),
                                                &subject_name)) {
-          std::cerr << "Could not extract name from certificate " << name
-                    << std::endl;
+          errors->push_back("Could not extract name from certificate " + name);
           return false;
         }
 
-        if (!MatchCertificateName(subject_name, name)) {
-          std::cerr << name << " is not a reasonable name for " << subject_name
-                    << std::endl;
+        if (!MatchCertificateName(subject_name, name, errors)) {
+          errors->push_back(name + " is not a reasonable name for " +
+                            subject_name);
           return false;
         }
 
@@ -393,8 +422,7 @@ bool ParseCertificatesFile(const std::string& certs_input, Pinsets* pinsets) {
         }
 
         if (!CalculateSPKIHashFromKey(buffer, &hash)) {
-          std::cerr << "Parsing of the public key " << name << " failed"
-                    << std::endl;
+          errors->push_back("Could not parse the public key for " + name);
           return false;
         }
 
@@ -410,14 +438,15 @@ bool ParseCertificatesFile(const std::string& certs_input, Pinsets* pinsets) {
 }
 
 // Checks if there are pins with the same name or the same hash.
-bool CheckForDuplicatePins(const Pinsets& pinsets) {
+bool CheckForDuplicatePins(const Pinsets& pinsets, ErrorList* errors) {
+  bool success = true;
   std::set<std::string> seen_names;
   std::map<std::string, std::string> seen_hashes;
 
   for (const auto& pin : pinsets.spki_hashes()) {
     if (seen_names.find(pin.first) != seen_names.cend()) {
-      std::cerr << "Duplicate pin name " << pin.first << std::endl;
-      return false;
+      errors->push_back("Duplicate pin name " + pin.first + " in pins file");
+      success = false;
     }
     seen_names.insert(pin.first);
 
@@ -425,19 +454,20 @@ bool CheckForDuplicatePins(const Pinsets& pinsets) {
         std::string(pin.second.data(), pin.second.data() + pin.second.size());
     std::map<std::string, std::string>::iterator it = seen_hashes.find(hash);
     if (it != seen_hashes.cend()) {
-      std::cerr << "Duplicate pin hash for " << pin.first
-                << ", already seen as " << it->second << std::endl;
-      return false;
+      errors->push_back("Duplicate pin hash for " + pin.first +
+                        ", already seen as " + it->second);
+      success = false;
     }
     seen_hashes.insert(std::pair<std::string, std::string>(hash, pin.first));
   }
 
-  return true;
+  return success;
 }
 
 // Checks if there are pinsets that reference non-existing pins, if two
 // pinsets share the same name, or if there are unused pins.
-bool CheckCertificatesInPinsets(const Pinsets& pinsets) {
+bool CheckCertificatesInPinsets(const Pinsets& pinsets, ErrorList* errors) {
+  bool success = true;
   std::set<std::string> pin_names;
   for (const auto& pin : pinsets.spki_hashes()) {
     pin_names.insert(pin.first);
@@ -447,9 +477,8 @@ bool CheckCertificatesInPinsets(const Pinsets& pinsets) {
   std::set<std::string> pinset_names;
   for (const auto& pinset : pinsets.pinsets()) {
     if (pinset_names.find(pinset.second->name()) != pinset_names.cend()) {
-      std::cerr << "Duplicate pinset name " << pinset.second->name()
-                << std::endl;
-      return false;
+      errors->push_back("Duplicate pinset name " + pinset.second->name());
+      success = false;
     }
     pinset_names.insert(pinset.second->name());
 
@@ -467,8 +496,10 @@ bool CheckCertificatesInPinsets(const Pinsets& pinsets) {
 
     for (const auto& pin_name : all_pin_names) {
       if (pin_names.find(pin_name) == pin_names.cend()) {
-        std::cerr << "Unknown pin: " << pin_name << std::endl;
-        return false;
+        errors->push_back("Pinset " + pinset.second->name() +
+                          " references pin " + pin_name +
+                          " which doesn't exist");
+        success = false;
       }
       used_pin_names.insert(pin_name);
     }
@@ -476,29 +507,33 @@ bool CheckCertificatesInPinsets(const Pinsets& pinsets) {
 
   for (const auto& pin_name : pin_names) {
     if (used_pin_names.find(pin_name) == used_pin_names.cend()) {
-      std::cerr << "Unused pin: " << pin_name << std::endl;
-      return false;
+      errors->push_back("Pin " + pin_name + " is unused.");
+      success = false;
     }
   }
 
-  return true;
+  return success;
 }
 
 // Checks if there are two or more entries for the same hostname.
-bool CheckDuplicateEntries(const TransportSecurityStateEntries& entries) {
+bool CheckDuplicateEntries(const TransportSecurityStateEntries& entries,
+                           ErrorList* errors) {
+  bool success = true;
   std::set<std::string> seen_entries;
   for (const auto& entry : entries) {
     if (seen_entries.find(entry->hostname) != seen_entries.cend()) {
-      std::cerr << "Duplicate entry for " << entry->hostname << std::endl;
-      return false;
+      errors->push_back("Duplicate entry for " + entry->hostname);
+      success = false;
     }
     seen_entries.insert(entry->hostname);
   }
-  return true;
+  return success;
 }
 
 // Checks for entries which have no effect.
-bool CheckNoopEntries(const TransportSecurityStateEntries& entries) {
+bool CheckNoopEntries(const TransportSecurityStateEntries& entries,
+                      ErrorList* errors) {
+  bool success = true;
   for (const auto& entry : entries) {
     if (!entry->force_https && entry->pinset.empty() && !entry->expect_ct &&
         !entry->expect_staple) {
@@ -507,28 +542,28 @@ bool CheckNoopEntries(const TransportSecurityStateEntries& entries) {
         continue;
       }
 
-      std::cerr
-          << "Entry for " + entry->hostname +
-                 " has no mode, no pins and is not expect-CT or expect-staple"
-          << std::endl;
-      return false;
+      errors->push_back(
+          "Entry for " + entry->hostname +
+          " has no mode, no pins and is not expect-CT or expect-staple");
+      success = false;
     }
   }
-  return true;
+  return success;
 }
 
 // Checks all entries for incorrect usage of the includeSubdomains flags.
-bool CheckSubdomainsFlags(const TransportSecurityStateEntries& entries) {
+bool CheckSubdomainsFlags(const TransportSecurityStateEntries& entries,
+                          ErrorList* errors) {
+  bool success = true;
   for (const auto& entry : entries) {
     if (entry->include_subdomains && entry->hpkp_include_subdomains) {
-      std::cerr << "Entry for \"" << entry->hostname
-                << "\" sets include_subdomains_for_pinning but also sets "
-                   "include_subdomains, which implies it"
-                << std::endl;
-      return false;
+      errors->push_back("Entry for " + entry->hostname +
+                        " sets include_subdomains_for_pinning but also sets "
+                        "include_subdomains, which implies it");
+      success = false;
     }
   }
-  return true;
+  return success;
 }
 
 }  // namespace
@@ -554,8 +589,6 @@ int main(int argc, char* argv[]) {
     return 1;
   }
 
-  bool verbose = command_line.HasSwitch("v");
-
   base::FilePath json_filepath = base::FilePath::FromUTF8Unsafe(argv[1]);
   if (!base::PathExists(json_filepath)) {
     std::cerr << "Input JSON file doesn't exist." << std::endl;
@@ -586,19 +619,21 @@ int main(int argc, char* argv[]) {
   Pinsets pinsets;
   DomainIDList domain_ids;
 
-  if (!ParseCertificatesFile(certs_input, &pinsets)) {
-    std::cerr << "Error while parsing the pins file." << std::endl;
-    return 1;
-  }
-  if (!ParseJSON(json_input, &entries, &pinsets, &domain_ids)) {
-    std::cerr << "Error while parsing the JSON file." << std::endl;
+  ErrorList errors;
+  if (!ParseCertificatesFile(certs_input, &pinsets, &errors) ||
+      !ParseJSON(json_input, &entries, &pinsets, &domain_ids, &errors)) {
+    std::cerr << "Error while parsing the input files." << std::endl;
+    PrintErrors(errors);
     return 1;
   }
 
-  if (!CheckDuplicateEntries(entries) || !CheckNoopEntries(entries) ||
-      !CheckSubdomainsFlags(entries) || !CheckForDuplicatePins(pinsets) ||
-      !CheckCertificatesInPinsets(pinsets)) {
+  if (!CheckDuplicateEntries(entries, &errors) ||
+      !CheckNoopEntries(entries, &errors) ||
+      !CheckSubdomainsFlags(entries, &errors) ||
+      !CheckForDuplicatePins(pinsets, &errors) ||
+      !CheckCertificatesInPinsets(pinsets, &errors)) {
     std::cerr << "Checks failed. Aborting." << std::endl;
+    PrintErrors(errors);
     return 1;
   }
 
@@ -615,19 +650,29 @@ int main(int argc, char* argv[]) {
     return 1;
   }
 
-  std::string result;
+  std::string output;
   PreloadedStateGenerator generator;
-  result = generator.Generate(preload_template, entries, domain_ids, pinsets,
-                              verbose);
+  if (!generator.Generate(preload_template, entries, domain_ids, pinsets,
+                          &output)) {
+    std::cerr << "Trie generation failed." << std::endl;
+    return 1;
+  }
 
   base::FilePath output_path;
   output_path = base::FilePath::FromUTF8Unsafe(argv[4]);
 
-  if (base::WriteFile(output_path, result.c_str(),
-                      static_cast<uint32_t>(result.size())) <= 0) {
+  if (base::WriteFile(output_path, output.c_str(),
+                      static_cast<uint32_t>(output.size())) <= 0) {
     std::cerr << "Failed to write output." << std::endl;
     return 1;
   }
 
+  if (command_line.HasSwitch("v")) {
+    std::cout << "Wrote trie containing " << entries.size()
+              << " entries, referencing " << pinsets.size() << " pinsets and "
+              << domain_ids.size() << " domain IDs to "
+              << output_path.AsUTF8Unsafe() << std::endl;
+  }
+
   return 0;
 }