Add fuzzer test for preg_parser

components/policy/core/common/preg_parser.cc

Index: components/policy/core/common/preg_parser.cc
diff --git a/components/policy/core/common/preg_parser.cc b/components/policy/core/common/preg_parser.cc
index 81261996f933bf0a77ca95dc5dfd2c6fcc3b677f..2872fb97b37f9ca6a83658b528434b4d56685337 100644
--- a/components/policy/core/common/preg_parser.cc
+++ b/components/policy/core/common/preg_parser.cc
@@ -24,10 +24,10 @@
 #include "base/strings/string16.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_byteorder.h"
 #include "base/values.h"
-#include "components/policy/core/common/policy_load_status.h"
 #include "components/policy/core/common/registry_dict.h"
 
 #if defined(OS_WIN)
@@ -216,8 +216,8 @@ void HandleRecord(const base::string16& key_name,
     return;
   }
 
-  std::string action_trigger(base::ToLowerASCII(value_name.substr(
-      arraysize(kActionTriggerPrefix) - 1)));
+  std::string action_trigger(base::ToLowerASCII(
+      value_name.substr(arraysize(kActionTriggerPrefix) - 1)));
   if (action_trigger == kActionTriggerDeleteValues) {
     for (const std::string& value :
          base::SplitString(DecodePRegStringValue(data), ";",
@@ -231,9 +231,8 @@ void HandleRecord(const base::string16& key_name,
       dict->RemoveKey(key);
   } else if (base::StartsWith(action_trigger, kActionTriggerDel,
                               base::CompareCase::SENSITIVE)) {
-  dict->RemoveValue(
-        value_name.substr(arraysize(kActionTriggerPrefix) - 1 +
-                          arraysize(kActionTriggerDel) - 1));
+    dict->RemoveValue(value_name.substr(arraysize(kActionTriggerPrefix) - 1 +
+                                        arraysize(kActionTriggerDel) - 1));
   } else if (base::StartsWith(action_trigger, kActionTriggerDelVals,
                               base::CompareCase::SENSITIVE)) {
     // Delete all values.
@@ -253,41 +252,60 @@ void HandleRecord(const base::string16& key_name,
 namespace policy {
 namespace preg_parser {
 
-const char kPRegFileHeader[8] =
-    { 'P', 'R', 'e', 'g', '\x01', '\x00', '\x00', '\x00' };
+const char kPRegFileHeader[8] = {'P',    'R',    'e',    'g',
+                                 '\x01', '\x00', '\x00', '\x00'};
 
 bool ReadFile(const base::FilePath& file_path,
               const base::string16& root,
               RegistryDict* dict,
-              PolicyLoadStatusSample* status) {
+              PolicyLoadStatusSample* status_sample) {
   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);
+    status_sample->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);
+  PolicyLoadStatus status = POLICY_LOAD_STATUS_SIZE;
+  bool res = ReadDataInternal(
+      mapped_file.data(), mapped_file.length(), root, dict, &status,
+      base::StringPrintf("file '%s'", file_path.value().c_str()));
+  if (!res) {
+    DCHECK(status != POLICY_LOAD_STATUS_SIZE);
+    status_sample->Add(status);
+  }
+  return res;
+}
+
+POLICY_EXPORT bool ReadDataInternal(const uint8_t* data,

[brucedawson, 2017/04/20 18:18:12]

Having a function argument named |data| is confusing when there is also (see
line 321) a local variable called data, with different type.

Consider renaming one of them to avoid future confusion?

It would be nice if we could make variable shadowing an error but there is too
much of it to make that practical at the moment.

+                                    size_t data_size,
+                                    const base::string16& root,
+                                    RegistryDict* dict,
+                                    PolicyLoadStatus* status,
+                                    const std::string& debug_name) {
+  DCHECK(status);
+
+  // Check data size.
+  if (data_size > kMaxPRegFileSize) {
+    LOG(ERROR) << "PReg " << debug_name << " too large: " << data_size;
+    *status = 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);
+  if (!data || data_size < kHeaderSize ||
+      memcmp(kPRegFileHeader, data, kHeaderSize) != 0) {
+    LOG(ERROR) << "Bad PReg " << debug_name;
+    *status = POLICY_LOAD_STATUS_PARSE_ERROR;
     return false;
   }
 
-  // Parse file contents, which is UCS-2 and little-endian. The latter I
+  // Parse data, which is expected to be 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_t* cursor = mapped_file.data() + kHeaderSize;
-  const uint8_t* end = mapped_file.data() + mapped_file.length();
+  const uint8_t* cursor = data + kHeaderSize;
+  const uint8_t* end = data + data_size;
   while (true) {
     if (cursor == end)
       return true;
@@ -341,10 +359,9 @@ bool ReadFile(const base::FilePath& file_path,
       HandleRecord(key_name.substr(root.size()), value, type, data, dict);
   }
 
-  LOG(ERROR) << "Error parsing " << file_path.value() << " at offset "
-             << reinterpret_cast<const uint8_t*>(cursor - 1) -
-                    mapped_file.data();
-  status->Add(POLICY_LOAD_STATUS_PARSE_ERROR);
+  LOG(ERROR) << "Error parsing PReg " << debug_name << " at offset "
+             << (reinterpret_cast<const uint8_t*>(cursor - 1) - data);
+  *status = POLICY_LOAD_STATUS_PARSE_ERROR;
   return false;
 }