Fix some bugs in the handling of dynamic crash keys.

chrome/app/breakpad_win.cc

 // Copyright (c) 2012 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/app/breakpad_win.h"
 
 #include <shellapi.h>
 #include <tchar.h>
 #include <userenv.h>
 #include <windows.h>
@@ skipping 741 matching lines @@
   }
 }
 
 extern "C" void __declspec(dllexport) __cdecl SetNumberOfViews(
     int number_of_views) {
   SetIntegerValue(g_num_of_views_offset, number_of_views);
 }
 
 void SetCrashKeyValue(const base::StringPiece& key,
                       const base::StringPiece& value) {
-  std::string key_string = key.as_string();
+  // 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.
+  base::StringPiece safe_key(key.substr(
+      0, google_breakpad::CustomInfoEntry::kNameMaxLength  - 1));
+  base::StringPiece safe_value(value.substr(
+      0, google_breakpad::CustomInfoEntry::kValueMaxLength - 1));

[Robert Sesek, 2013/04/16 17:54:29]

The crash_logging system should be doing this truncation for you. Though I
wouldn't object to doing it here as well for extra sanity.

[Roger McFarlane (Chromium), 2013/04/16 18:18:23]

What I've observed is that if you get down to entry->set(...) below and the
strings don't fit into the space allocated in CustomInfoEntry then you get a
crash because:

  - CustomInfoEntry::set* calls WindowsStringUtils::safe_wcscpy
  - WindowsStringUtils::safe_wcscpy calls wcscpy_s
  - wcscpy_s invokes the invalid parameter handler
  - the invalid parameter handler calls back into breakpad
  - breakpad triggers a crash

 
+  // Keep a copy of the safe key as a std::string, we'll reuse it later.
+  std::string key_string(safe_key.begin(), safe_key.end());
+
+  // 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.
   DynamicEntriesMap::iterator it = g_dynamic_entries->find(key_string);
   google_breakpad::CustomInfoEntry* entry = NULL;
   if (it == g_dynamic_entries->end()) {
-    if (g_dynamic_keys_offset >= g_dynamic_entries_count)
+    if (g_dynamic_entries->size() >= g_dynamic_entries_count)
       return;
     entry = &(*g_custom_entries)[g_dynamic_keys_offset++];
     g_dynamic_entries->insert(std::make_pair(key_string, entry));
   } else {
     entry = it->second;
   }
 
-  entry->set(UTF8ToWide(key).data(), UTF8ToWide(value).data());
+  entry->set(UTF8ToWide(safe_key).data(), UTF8ToWide(safe_value).data());
 }
 
 extern "C" void __declspec(dllexport) __cdecl SetCrashKeyValuePair(
     const char* key, const char* value) {
   SetCrashKeyValue(base::StringPiece(key), base::StringPiece(value));
 }
 
 void ClearCrashKeyValue(const base::StringPiece& key) {
   DynamicEntriesMap::iterator it = g_dynamic_entries->find(key.as_string());
   if (it == g_dynamic_entries->end())
@@ skipping 344 matching lines @@
   previous_filter = SetUnhandledExceptionFilter(filter);
 }
 
 void StringVectorToCStringVector(const std::vector<std::wstring>& wstrings,
                                  std::vector<const wchar_t*>* cstrings) {
   cstrings->clear();
   cstrings->reserve(wstrings.size());
   for (size_t i = 0; i < wstrings.size(); ++i)
     cstrings->push_back(wstrings[i].c_str());
 }