Add fuzzer test for preg_parser

components/policy/core/common/preg_parser.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 "components/policy/core/common/preg_parser.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <functional>
 #include <iterator>
 #include <limits>
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/files/file_path.h"
 #include "base/files/memory_mapped_file.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #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)
 #include "windows.h"
 #else
 // Registry data type constants.
 #define REG_NONE 0
 #define REG_SZ 1
 #define REG_EXPAND_SZ 2
 #define REG_BINARY 3
@@ skipping 168 matching lines @@
 
   std::string value_name(base::UTF16ToUTF8(value));
   if (!base::StartsWith(value_name, kActionTriggerPrefix,
                         base::CompareCase::SENSITIVE)) {
     std::unique_ptr<base::Value> value;
     if (DecodePRegValue(type, data, &value))
       dict->SetValue(value_name, std::move(value));
     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), ";",
                            base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY))
       dict->RemoveValue(value);
   } else if (base::StartsWith(action_trigger, kActionTriggerDeleteKeys,
                               base::CompareCase::SENSITIVE)) {
     for (const std::string& key :
          base::SplitString(DecodePRegStringValue(data), ";",
                            base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY))
       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.
     dict->ClearValues();
   } else if (base::StartsWith(action_trigger, kActionTriggerSecureKey,
                               base::CompareCase::SENSITIVE) ||
              base::StartsWith(action_trigger, kActionTriggerSoft,
                               base::CompareCase::SENSITIVE)) {
     // Doesn't affect values.
   } else {
     LOG(ERROR) << "Bad action trigger " << value_name;
   }
 }
 
 }  // namespace
 
 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;
 
     if (NextChar(&cursor, end) != kDelimBracketOpen)
       break;
 
     // Read the record fields.
     base::string16 key_name;
     base::string16 value;
@@ skipping 33 matching lines @@
     }
 
     if (current != kDelimBracketClose)
       break;
 
     // Process the record if it is within the |root| subtree.
     if (base::StartsWith(key_name, root, base::CompareCase::INSENSITIVE_ASCII))
       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;
 }
 
 }  // namespace preg_parser
 }  // namespace policy