Refactor the breakpad Windows key/value stash, step one.

components/breakpad/app/breakpad_win.cc

 // Copyright 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/breakpad/app/breakpad_win.h"
 
 #include <windows.h>
 #include <shellapi.h>
 #include <tchar.h>
 #include <userenv.h>
 #include <winnt.h>
 
 #include <algorithm>
+#include <map>
 #include <vector>
 
 #include "base/base_switches.h"
 #include "base/basictypes.h"
 #include "base/command_line.h"
 #include "base/debug/crash_logging.h"
 #include "base/debug/dump_without_crashing.h"
 #include "base/environment.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
@@ skipping 14 matching lines @@
 #include "sandbox/win/src/sidestep/preamble_patcher.h"
 
 // userenv.dll is required for GetProfileType().
 #pragma comment(lib, "userenv.lib")
 
 #pragma intrinsic(_AddressOfReturnAddress)
 #pragma intrinsic(_ReturnAddress)
 
 namespace breakpad {
 
-std::vector<google_breakpad::CustomInfoEntry>* g_custom_entries = NULL;
+// Manages the breakpad key/value pair stash, there may only be one instance
+// of this class per process at one time.
+// Exposed for testing only.
+class KeyValuePairKeeper {

[Robert Sesek, 2014/05/29 20:34:41]

Not quite a fan of this name. Perhaps CustomInfoMap (since it contains
CustomInfoEntry's)? Or CrashKeysWin?

[Sigurður Ásgeirsson, 2014/05/30 15:14:49]

Yeah, me neither :/. You know of course that there are only two hard problems in
software development:
1. Cache eviction.
2. Naming.
3. One-offs.
I like CrashKeysWin, so done, thanks.

+ public:
+  KeyValuePairKeeper();
+  ~KeyValuePairKeeper();
+
+  // May only be called once.
+  google_breakpad::CustomClientInfo*
+     GetCustomInfo(const std::wstring& exe_path, const std::wstring& type);
+
+  void SetCrashKeyValue(const std::wstring& key, const std::wstring& value);
+  void ClearCrashKeyValue(const std::wstring& key);
+
+  static KeyValuePairKeeper* keeper() { return keeper_; }
+
+ private:
+  // Sets private

[Robert Sesek, 2014/05/29 20:34:41]

Incomplete comment?

[Sigurður Ásgeirsson, 2014/05/30 15:14:49]

Done.

+  void SetPluginPath(const std::wstring& path);
+  void SetBreakpadDumpPath();
+
+  // Must not be resized after GetCustomInfo is invoked.
+  std::vector<google_breakpad::CustomInfoEntry> custom_entries_;
+
+  typedef std::map<std::wstring, google_breakpad::CustomInfoEntry*>
+      DynamicEntriesMap;
+  base::Lock lock_;
+  // Keeps track of the next index for a new dynamic entry.
+  size_t dynamic_keys_offset_;  // Under lock_.
+  // Maintains key->entry information for dynamic key/value entries
+  // in custom_entries_.
+  DynamicEntriesMap dynamic_entries_;  // Under lock_.
+
+  // Stores the sole instance of this class allowed per process.
+  static KeyValuePairKeeper* keeper_;
+};
+
+KeyValuePairKeeper* KeyValuePairKeeper::keeper_;
+
+KeyValuePairKeeper::KeyValuePairKeeper() : dynamic_keys_offset_(0) {
+  DCHECK_EQ(static_cast<KeyValuePairKeeper*>(NULL), keeper_);
+

[Robert Sesek, 2014/05/29 20:34:41]

nit: remove blank line

[Sigurður Ásgeirsson, 2014/05/30 15:14:49]

Done.

+  keeper_ = this;
+}
+
+KeyValuePairKeeper::~KeyValuePairKeeper() {
+  DCHECK_EQ(this, keeper_);
+  keeper_ = NULL;
+}
 
 namespace {
 
 // Minidump with stacks, PEB, TEB, and unloaded module list.
 const MINIDUMP_TYPE kSmallDumpType = static_cast<MINIDUMP_TYPE>(
     MiniDumpWithProcessThreadData |  // Get PEB and TEB.
     MiniDumpWithUnloadedModules);  // Get unloaded modules when available.
 
 // Minidump with all of the above, plus memory referenced from stack.
 const MINIDUMP_TYPE kLargerDumpType = static_cast<MINIDUMP_TYPE>(
@@ skipping 20 matching lines @@
 google_breakpad::ExceptionHandler* g_dumphandler_no_crash = NULL;
 
 EXCEPTION_POINTERS g_surrogate_exception_pointers = {0};
 EXCEPTION_RECORD g_surrogate_exception_record = {0};
 CONTEXT g_surrogate_context = {0};
 
 typedef NTSTATUS (WINAPI* NtTerminateProcessPtr)(HANDLE ProcessHandle,
                                                  NTSTATUS ExitStatus);
 char* g_real_terminate_process_stub = NULL;
 
-base::Lock* g_dynamic_entries_lock = NULL;
-// Under *g_dynamic_entries_lock.
-size_t g_dynamic_keys_offset = 0;
-typedef std::map<std::wstring, google_breakpad::CustomInfoEntry*>
-    DynamicEntriesMap;
-// Under *g_dynamic_entries_lock.
-DynamicEntriesMap* g_dynamic_entries = NULL;
 
 // Allow for 256 dynamic entries in addition to the fixed set of entries
 // set up in this file.
 const size_t kMaxDynamicEntries = 256;
 
 // Maximum length for plugin path to include in plugin crash reports.
 const size_t kMaxPluginPathLength = 256;
 
+}  // namespace
+
 // Dumps the current process memory.
 extern "C" void __declspec(dllexport) __cdecl DumpProcess() {
   if (g_breakpad) {
     g_breakpad->WriteMinidump();
   }
 }
 
 // Used for dumping a process state when there is no crash.
 extern "C" void __declspec(dllexport) __cdecl DumpProcessWithoutCrash() {
   if (g_dumphandler_no_crash) {
     g_dumphandler_no_crash->WriteMinidump();
   }
 }
 
+namespace {
+
 // We need to prevent ICF from folding DumpForHangDebuggingThread() and
 // DumpProcessWithoutCrashThread() together, since that makes them
 // indistinguishable in crash dumps. We do this by making the function
 // bodies unique, and prevent optimization from shuffling things around.
 MSVC_DISABLE_OPTIMIZE()
 MSVC_PUSH_DISABLE_WARNING(4748)
 
 DWORD WINAPI DumpProcessWithoutCrashThread(void*) {
   DumpProcessWithoutCrash();
   return 0;
 }
 
 // The following two functions do exactly the same thing as the two above. But
 // we want the signatures to be different so that we can easily track them in
 // crash reports.
 // TODO(yzshen): Remove when enough information is collected and the hang rate
 // of pepper/renderer processes is reduced.
 DWORD WINAPI DumpForHangDebuggingThread(void*) {
   DumpProcessWithoutCrash();
   VLOG(1) << "dumped for hang debugging";
   return 0;
 }
 
 MSVC_POP_WARNING()
 MSVC_ENABLE_OPTIMIZE()
 
+}  // namespace
+
 // Injects a thread into a remote process to dump state when there is no crash.
 extern "C" HANDLE __declspec(dllexport) __cdecl
 InjectDumpProcessWithoutCrash(HANDLE process) {
   return CreateRemoteThread(process, NULL, 0, DumpProcessWithoutCrashThread,
                             0, 0, NULL);
 }
 
 extern "C" HANDLE __declspec(dllexport) __cdecl
 InjectDumpForHangDebugging(HANDLE process) {
   return CreateRemoteThread(process, NULL, 0, DumpForHangDebuggingThread,
                             0, 0, NULL);
 }
 
 // Appends the plugin path to |g_custom_entries|.
-void SetPluginPath(const std::wstring& path) {
-  DCHECK(g_custom_entries);
-
+void KeyValuePairKeeper::SetPluginPath(const std::wstring& path) {
   if (path.size() > kMaxPluginPathLength) {
     // If the path is too long, truncate from the start rather than the end,
     // since we want to be able to recover the DLL name.
     SetPluginPath(path.substr(path.size() - kMaxPluginPathLength));
     return;
   }
 
   // The chunk size without terminator.
   const size_t kChunkSize = static_cast<size_t>(
       google_breakpad::CustomInfoEntry::kValueMaxLength - 1);
 
   int chunk_index = 0;
   size_t chunk_start = 0;  // Current position inside |path|
 
   for (chunk_start = 0; chunk_start < path.size(); chunk_index++) {
     size_t chunk_length = std::min(kChunkSize, path.size() - chunk_start);
 
-    g_custom_entries->push_back(google_breakpad::CustomInfoEntry(
+    custom_entries_.push_back(google_breakpad::CustomInfoEntry(
         base::StringPrintf(L"plugin-path-chunk-%i", chunk_index + 1).c_str(),
         path.substr(chunk_start, chunk_length).c_str()));
 
     chunk_start += chunk_length;
   }
 }
 
 // Appends the breakpad dump path to |g_custom_entries|.
-void SetBreakpadDumpPath() {
-  DCHECK(g_custom_entries);
+void KeyValuePairKeeper::SetBreakpadDumpPath() {
   base::FilePath crash_dumps_dir_path;
   if (GetBreakpadClient()->GetAlternativeCrashDumpLocation(
           &crash_dumps_dir_path)) {
-    g_custom_entries->push_back(google_breakpad::CustomInfoEntry(
+    custom_entries_.push_back(google_breakpad::CustomInfoEntry(
         L"breakpad-dump-location", crash_dumps_dir_path.value().c_str()));
   }
 }
 
 // Returns a string containing a list of all modifiers for the loaded profile.
 std::wstring GetProfileType() {
   std::wstring profile_type;
   DWORD profile_bits = 0;
   if (::GetProfileType(&profile_bits)) {
     static const struct {
@@ skipping 15 matching lines @@
     }
   } else {
     DWORD last_error = ::GetLastError();
     base::SStringPrintf(&profile_type, L"error %u", last_error);
   }
   return profile_type;
 }
 
 // Returns the custom info structure based on the dll in parameter and the
 // process type.
-google_breakpad::CustomClientInfo* GetCustomInfo(const std::wstring& exe_path,
-                                                 const std::wstring& type) {
+google_breakpad::CustomClientInfo*
+KeyValuePairKeeper::GetCustomInfo(const std::wstring& exe_path,
+                                  const std::wstring& type) {
   base::string16 version, product;
   base::string16 special_build;
   base::string16 channel_name;
   GetBreakpadClient()->GetProductNameAndVersion(
       base::FilePath(exe_path),
       &product,
       &version,
       &special_build,
       &channel_name);
 
   // We only expect this method to be called once per process.
-  DCHECK(!g_custom_entries);
-  g_custom_entries = new std::vector<google_breakpad::CustomInfoEntry>;
-
-  // Common g_custom_entries.
-  g_custom_entries->push_back(
+  // Common enties
+  custom_entries_.push_back(
       google_breakpad::CustomInfoEntry(L"ver",
                                        base::UTF16ToWide(version).c_str()));
-  g_custom_entries->push_back(
+  custom_entries_.push_back(
       google_breakpad::CustomInfoEntry(L"prod",
                                        base::UTF16ToWide(product).c_str()));
-  g_custom_entries->push_back(
+  custom_entries_.push_back(
       google_breakpad::CustomInfoEntry(L"plat", L"Win32"));
-  g_custom_entries->push_back(
+  custom_entries_.push_back(
       google_breakpad::CustomInfoEntry(L"ptype", type.c_str()));
-  g_custom_entries->push_back(google_breakpad::CustomInfoEntry(
+  custom_entries_.push_back(google_breakpad::CustomInfoEntry(
       L"pid", base::StringPrintf(L"%d", ::GetCurrentProcessId()).c_str()));
-  g_custom_entries->push_back(google_breakpad::CustomInfoEntry(
+  custom_entries_.push_back(google_breakpad::CustomInfoEntry(
       L"channel", base::UTF16ToWide(channel_name).c_str()));
-  g_custom_entries->push_back(google_breakpad::CustomInfoEntry(
+  custom_entries_.push_back(google_breakpad::CustomInfoEntry(
       L"profile-type", GetProfileType().c_str()));
 
   if (!special_build.empty())
-    g_custom_entries->push_back(google_breakpad::CustomInfoEntry(
+    custom_entries_.push_back(google_breakpad::CustomInfoEntry(
         L"special", base::UTF16ToWide(special_build).c_str()));
 
   if (type == L"plugin" || type == L"ppapi") {
     std::wstring plugin_path =
         CommandLine::ForCurrentProcess()->GetSwitchValueNative("plugin-path");
     if (!plugin_path.empty())
       SetPluginPath(plugin_path);
   }
 
   // Check whether configuration management controls crash reporting.
   bool crash_reporting_enabled = true;
   bool controlled_by_policy = GetBreakpadClient()->ReportingIsEnforcedByPolicy(
       &crash_reporting_enabled);
   const CommandLine& command = *CommandLine::ForCurrentProcess();
   bool use_crash_service =
       !controlled_by_policy && (command.HasSwitch(switches::kNoErrorDialogs) ||
                                 GetBreakpadClient()->IsRunningUnattended());
   if (use_crash_service)
     SetBreakpadDumpPath();
 
   // Create space for dynamic ad-hoc keys. The names and values are set using
   // the API defined in base/debug/crash_logging.h.
-  g_dynamic_keys_offset = g_custom_entries->size();
+  dynamic_keys_offset_ = custom_entries_.size();
   for (size_t i = 0; i < kMaxDynamicEntries; ++i) {
     // The names will be mutated as they are set. Un-numbered since these are
     // merely placeholders. The name cannot be empty because Breakpad's
     // HTTPUpload will interpret that as an invalid parameter.
-    g_custom_entries->push_back(
+    custom_entries_.push_back(
         google_breakpad::CustomInfoEntry(L"unspecified-crash-key", L""));
   }
 
-  g_dynamic_entries_lock = new base::Lock;
-  g_dynamic_entries = new DynamicEntriesMap;
-
   static google_breakpad::CustomClientInfo custom_client_info;
-  custom_client_info.entries = &g_custom_entries->front();
-  custom_client_info.count = g_custom_entries->size();
+  custom_client_info.entries = &custom_entries_.front();
+  custom_client_info.count = custom_entries_.size();
 
   return &custom_client_info;
 }
 
+namespace {
+
 // This callback is used when we want to get a dump without crashing the
 // process.
 bool DumpDoneCallbackWhenNoCrash(const wchar_t*, const wchar_t*, void*,
                                  EXCEPTION_POINTERS* ex_info,
                                  MDRawAssertionInfo*, bool) {
   return true;
 }
 
 // This callback is executed when the browser process has crashed, after
 // the crash dump has been created. We need to minimize the amount of work
@@ skipping 69 matching lines @@
 }
 
 // Exception filter for the service process used when breakpad is not enabled.
 // We just display the "Do you want to restart" message and then die
 // (without calling the previous filter).
 long WINAPI ServiceExceptionFilter(EXCEPTION_POINTERS* info) {
   DumpDoneCallback(NULL, NULL, NULL, info, NULL, false);
   return EXCEPTION_EXECUTE_HANDLER;
 }
 
-// NOTE: This function is used by SyzyASAN to annotate crash reports. If you
-// change the name or signature of this function you will break SyzyASAN
-// instrumented releases of Chrome. Please contact syzygy-team@chromium.org
-// before doing so!
-extern "C" void __declspec(dllexport) __cdecl SetCrashKeyValueImpl(
-    const wchar_t* key, const wchar_t* value) {
-  if (!g_dynamic_entries)
-    return;
+}  // namespace
 
+void KeyValuePairKeeper::SetCrashKeyValue(
+    const std::wstring& key, const std::wstring& value) {
   // CustomInfoEntry limits the length of key and value. If they exceed
   // their maximum length the underlying string handling functions raise
   // an exception and prematurely trigger a crash. Truncate here.
   std::wstring safe_key(std::wstring(key).substr(
       0, google_breakpad::CustomInfoEntry::kNameMaxLength  - 1));
   std::wstring safe_value(std::wstring(value).substr(
       0, google_breakpad::CustomInfoEntry::kValueMaxLength - 1));
 
   // If we already have a value for this key, update it; otherwise, insert
   // the new value if we have not exhausted the pre-allocated slots for dynamic
   // entries.
-  DCHECK(g_dynamic_entries_lock);
-  base::AutoLock lock(*g_dynamic_entries_lock);
+  base::AutoLock lock(lock_);
 
-  DynamicEntriesMap::iterator it = g_dynamic_entries->find(safe_key);
+  DynamicEntriesMap::iterator it = dynamic_entries_.find(safe_key);
   google_breakpad::CustomInfoEntry* entry = NULL;
-  if (it == g_dynamic_entries->end()) {
-    if (g_dynamic_entries->size() >= kMaxDynamicEntries)
+  if (it == dynamic_entries_.end()) {
+    if (dynamic_entries_.size() >= kMaxDynamicEntries)
       return;
-    entry = &(*g_custom_entries)[g_dynamic_keys_offset++];
-    g_dynamic_entries->insert(std::make_pair(safe_key, entry));
+    entry = &custom_entries_[dynamic_keys_offset_++];
+    dynamic_entries_.insert(std::make_pair(safe_key, entry));
   } else {
     entry = it->second;
   }
 
   entry->set(safe_key.data(), safe_value.data());
 }
 
-extern "C" void __declspec(dllexport) __cdecl ClearCrashKeyValueImpl(
-    const wchar_t* key) {
-  if (!g_dynamic_entries)
+// NOTE: This function is used by SyzyASAN to annotate crash reports. If you
+// change the name or signature of this function you will break SyzyASAN
+// instrumented releases of Chrome. Please contact syzygy-team@chromium.org
+// before doing so!
+extern "C" void __declspec(dllexport) __cdecl SetCrashKeyValueImpl(
+    const wchar_t* key, const wchar_t* value) {
+  KeyValuePairKeeper* keeper = KeyValuePairKeeper::keeper();
+  if (!keeper)
     return;
 
-  DCHECK(g_dynamic_entries_lock);
-  base::AutoLock lock(*g_dynamic_entries_lock);
+  // TODO(siggi): This doesn't look quite right - there's NULL deref potential
+  //    here, and an implicit std::wstring conversion. Fixme.
+  keeper->SetCrashKeyValue(key, value);
+}
+
+void KeyValuePairKeeper::ClearCrashKeyValue(const std::wstring& key) {
+  base::AutoLock lock(lock_);
 
   std::wstring key_string(key);
-  DynamicEntriesMap::iterator it = g_dynamic_entries->find(key_string);
-  if (it == g_dynamic_entries->end())
+  DynamicEntriesMap::iterator it = dynamic_entries_.find(key_string);
+  if (it == dynamic_entries_.end())
     return;
 
   it->second->set_value(NULL);
 }
 
-}  // namespace
+extern "C" void __declspec(dllexport) __cdecl ClearCrashKeyValueImpl(
+    const wchar_t* key) {
+  KeyValuePairKeeper* keeper = KeyValuePairKeeper::keeper();
+  if (!keeper)
+    return;
+
+  // TODO(siggi): This doesn't look quite right - there's NULL deref potential
+  //    here, and an implicit std::wstring conversion. Fixme.

[Robert Sesek, 2014/05/29 20:34:41]

Where's the NULL deref potential?

[Sigurður Ásgeirsson, 2014/05/30 15:14:49]

If there's NULL on the input, std::string will try to deref it, I believe. As
this is an exported function, it might be better to be defensive here, but as-is
this behaves much as before.

+  keeper->ClearCrashKeyValue(key);
+}
 
 static bool WrapMessageBoxWithSEH(const wchar_t* text, const wchar_t* caption,
                                   UINT flags, bool* exit_now) {
   // We wrap the call to MessageBoxW with a SEH handler because it some
   // machines with CursorXP, PeaDict or with FontExplorer installed it crashes
   // uncontrollably here. Being this a best effort deal we better go away.
   __try {
     *exit_now = (IDOK != ::MessageBoxW(NULL, text, caption, flags));
   } __except(EXCEPTION_EXECUTE_HANDLER) {
     // Its not safe to continue executing, exit silently here.
@@ skipping 190 matching lines @@
   if (process_type.empty())
     process_type = L"browser";
 
   wchar_t exe_path[MAX_PATH];
   exe_path[0] = 0;
   GetModuleFileNameW(NULL, exe_path, MAX_PATH);
 
   bool is_per_user_install =
       GetBreakpadClient()->GetIsPerUserInstall(base::FilePath(exe_path));
 
+  // This is intentionally leaked.
+  KeyValuePairKeeper* keeper = new KeyValuePairKeeper();
+
   google_breakpad::CustomClientInfo* custom_info =
-      GetCustomInfo(exe_path, process_type);
+      keeper->GetCustomInfo(exe_path, process_type);
 
   google_breakpad::ExceptionHandler::MinidumpCallback callback = NULL;
   LPTOP_LEVEL_EXCEPTION_FILTER default_filter = NULL;
   // We install the post-dump callback only for the browser and service
   // processes. It spawns a new browser/service process.
   if (process_type == L"browser") {
     callback = &DumpDoneCallback;
     default_filter = &ChromeExceptionFilter;
   } else if (process_type == L"service") {
     callback = &DumpDoneCallback;
@@ skipping 66 matching lines @@
     if (process_type != L"browser" &&
         !GetBreakpadClient()->IsRunningUnattended()) {
       // Initialize the hook TerminateProcess to catch unexpected exits.
       InitTerminateProcessHooks();
     }
 #endif
   }
 }
 
 }  // namespace breakpad