Add UMA histograms for policy loading.

chrome/browser/policy/preg_parser_win.cc

 // Copyright (c) 2013 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 "chrome/browser/policy/preg_parser_win.h"
 
 #include <windows.h>
 
 #include <algorithm>
 #include <iterator>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/files/file_path.h"
 #include "base/files/memory_mapped_file.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/string16.h"
 #include "base/string_util.h"
 #include "base/sys_byteorder.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
+#include "chrome/browser/policy/policy_load_status.h"
 
 namespace policy {
 namespace preg_parser {
 
 const char kPRegFileHeader[8] =
     { 'P', 'R', 'e', 'g', '\x01', '\x00', '\x00', '\x00' };

[Joao da Silva, 2013/04/16 19:32:57]

Why did you revert this in the 2nd patch set?

[Mattias Nissler (ping if slow), 2013/04/17 10:16:35]

I didn't revert. The second patch set is rebased onto ToT which doesn't have the
other commit that adds this yet.

 
 // Maximum PReg file size we're willing to accept.
 const int64 kMaxPRegFileSize = 1024 * 1024 * 16;
 
 // Constants for PReg file delimiters.
 const char16 kDelimBracketOpen = L'[';
 const char16 kDelimBracketClose = L']';
 const char16 kDelimSemicolon = L';';
 
 // Registry path separator.
@@ skipping 169 matching lines @@
   } else if (StartsWithASCII(action_trigger, kActionTriggerSecureKey, true) ||
              StartsWithASCII(action_trigger, kActionTriggerSoft, true)) {
     // Doesn't affect values.
   } else {
     LOG(ERROR) << "Bad action trigger " << value_name;
   }
 }
 
 bool ReadFile(const base::FilePath& file_path,
               const string16& root,
-              base::DictionaryValue* dict) {
+              base::DictionaryValue* dict,
+              PolicyLoadStatusSample* status) {
   base::MemoryMappedFile mapped_file;
   if (!mapped_file.Initialize(file_path) || !mapped_file.IsValid()) {
     PLOG(ERROR) << "Failed to map " << file_path.value();
+    status->Add(POLICY_LOAD_STATUS_READ_ERROR);
     return false;
   }
 
   if (mapped_file.length() > kMaxPRegFileSize) {
     LOG(ERROR) << "PReg file " << file_path.value() << " too large: "
                << mapped_file.length();
+    status->Add(POLICY_LOAD_STATUS_TOO_BIG);
     return false;
   }
 
   // Check the header.
   const int kHeaderSize = arraysize(kPRegFileHeader);
   if (mapped_file.length() < kHeaderSize ||
       memcmp(kPRegFileHeader, mapped_file.data(), kHeaderSize) != 0) {
     LOG(ERROR) << "Bad policy file " << file_path.value();
+    status->Add(POLICY_LOAD_STATUS_PARSE_ERROR);
     return false;
   }
 
   // Parse file contents, which is UCS-2 and little-endian. The latter I
   // couldn't find documentation on, but the example I saw were all
   // little-endian. It'd be interesting to check on big-endian hardware.
   const uint8* cursor = mapped_file.data() + kHeaderSize;
   const uint8* end = mapped_file.data() + mapped_file.length();
   while (true) {
     if (cursor == end)
@@ skipping 27 matching lines @@
 
     if (current == kDelimSemicolon) {
       if (!ReadField32(&cursor, end, &size))
         break;
       current = NextChar(&cursor, end);
     }
 
     if (current == kDelimSemicolon) {
       if (size > kMaxPRegFileSize)
         break;
       data.resize(size);

[Joao da Silva, 2013/04/16 17:31:04]

This escaped the first code review: my concern back then was that |size| is a
wrong value, and resizing |data| blows up the memory consumption. I wouldn't
enforce a file size limit (that's up to the administrator), but I'd enforce a
buffer size limit here.

A broken Registry.pol file with a huge size field here would always crash chrome
on startup.

[Mattias Nissler (ping if slow), 2013/04/16 18:51:14]

The kMaxPRegFileSize check two lines above should catch this situation? Or are
you worried about the 16M buffer?

[Joao da Silva, 2013/04/16 19:32:57]

Looking at this again I now see the size check in line 286, that somehow escaped
my attention earlier. Please ignore.

       if (!ReadFieldBinary(&cursor, end, size, vector_as_array(&data)))
         break;
       current = NextChar(&cursor, end);
     }
 
     if (current != kDelimBracketClose)
       break;
 
     // Process the record if it is within the |root| subtree.
     if (StartsWith(key_name, root, false))
       HandleRecord(key_name.substr(root.size()), value, type, data, dict);
   }
 
   LOG(ERROR) << "Error parsing " << file_path.value() << " at offset "
              << reinterpret_cast<const uint8*>(cursor - 1) - mapped_file.data();
+  status->Add(POLICY_LOAD_STATUS_PARSE_ERROR);
   return false;
 }
 
 }  // namespace preg_parser
 }  // namespace policy