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

components/breakpad/app/breakpad_win.cc

Index: components/breakpad/app/breakpad_win.cc
diff --git a/components/breakpad/app/breakpad_win.cc b/components/breakpad/app/breakpad_win.cc
index fec106581a5bc202afe86d9fc01344f52ba43963..da760a1ee3f3f76fc19a2fde0181150b4b4fe2ab 100644
--- a/components/breakpad/app/breakpad_win.cc
+++ b/components/breakpad/app/breakpad_win.cc
@@ -11,6 +11,7 @@
 #include <winnt.h>
 
 #include <algorithm>
+#include <map>
 #include <vector>
 
 #include "base/base_switches.h"
@@ -45,7 +46,56 @@
 
 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 {
 
@@ -86,13 +136,6 @@ 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.
@@ -101,6 +144,8 @@ 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) {
@@ -115,6 +160,8 @@ extern "C" void __declspec(dllexport) __cdecl DumpProcessWithoutCrash() {
   }
 }
 
+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
@@ -141,6 +188,8 @@ DWORD WINAPI DumpForHangDebuggingThread(void*) {
 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) {
@@ -155,9 +204,7 @@ InjectDumpForHangDebugging(HANDLE process) {
 }
 
 // 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.
@@ -175,7 +222,7 @@ void SetPluginPath(const std::wstring& 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()));
 
@@ -184,12 +231,11 @@ void SetPluginPath(const std::wstring& path) {
 }
 
 // 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()));
   }
 }
@@ -225,8 +271,9 @@ std::wstring GetProfileType() {
 
 // 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;
@@ -238,29 +285,26 @@ google_breakpad::CustomClientInfo* GetCustomInfo(const std::wstring& exe_path,
       &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") {
@@ -283,25 +327,24 @@ google_breakpad::CustomClientInfo* GetCustomInfo(const std::wstring& exe_path,
 
   // 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*,
@@ -391,15 +434,10 @@ long WINAPI ServiceExceptionFilter(EXCEPTION_POINTERS* info) {
   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.
@@ -411,16 +449,15 @@ extern "C" void __declspec(dllexport) __cdecl SetCrashKeyValueImpl(
   // 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;
   }
@@ -428,23 +465,42 @@ extern "C" void __declspec(dllexport) __cdecl SetCrashKeyValueImpl(
   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) {
@@ -655,8 +711,11 @@ void InitCrashReporter(const std::string& process_type_switch) {
   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;