Add WebView-specific whitelist for crash keys.

components/crash/content/app/breakpad_linux.cc

Index: components/crash/content/app/breakpad_linux.cc
diff --git a/components/crash/content/app/breakpad_linux.cc b/components/crash/content/app/breakpad_linux.cc
index 3c31d7619e49bf56210ff310e3a73905c58a43ec..29480131e43f1a47fefe4963d0d0d00f55862ba0 100644
--- a/components/crash/content/app/breakpad_linux.cc
+++ b/components/crash/content/app/breakpad_linux.cc
@@ -23,6 +23,7 @@
 #include <unistd.h>
 
 #include <algorithm>
+#include <map>
 #include <string>
 
 #include "base/base_switches.h"
@@ -30,6 +31,7 @@
 #include "base/debug/crash_logging.h"
 #include "base/debug/dump_without_crashing.h"
 #include "base/files/file_path.h"
+#include "base/format_macros.h"
 #include "base/lazy_instance.h"
 #include "base/linux_util.h"
 #include "base/macros.h"
@@ -39,6 +41,7 @@
 #include "base/process/memory.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/threading/thread_checker.h"
 #include "breakpad/src/client/linux/crash_generation/crash_generation_client.h"
 #include "breakpad/src/client/linux/handler/exception_handler.h"
@@ -1098,9 +1101,60 @@ void EnableNonBrowserCrashDumping() {
 }
 #endif  // defined(OS_ANDROID)
 
+std::unordered_set<std::string> g_crash_keys_whitelist;

[gsennton, 2017/02/27 18:31:37]

I'm not sure what constraints we have on what data-structures to use here, any
thoughts?

[Robert Sesek, 2017/02/27 23:15:28]

Basically nothing that allocates, if possible. Creating an iterator here may, so
I wouldn't use this. A NULL-terminated C array is probably safest.

[gsennton, 2017/02/28 21:02:42]

This should be fine now :).

[Tobias Sargeant, 2017/02/28 22:18:55]

I think Robert's suggestion of a NULL terminated C array is safest (I expect
that std::vector won't allocate given the usage below, but I don't know for
sure, and I'm not sure that that guarantee is given in the STL spec).

However, we don't access this from a compromised context (Robert: I'm assuming
that keys are only set prior to a crash. Is that correct) so I think that the
unordered_set should be ok?

[Robert Sesek, 2017/03/02 15:00:38]

No, crash keys can be set in a compromised context.

[gsennton, 2017/03/02 16:18:01]

I changed the vector to a NULL terminated C array now :)

+
+// TODO copy-pasted from base/debug/crash_logging.cc, can we just BASE_EXPORT
+// that implementation instead?

[gsennton, 2017/02/27 18:31:38]

^^^ is BASE_EXPORTING NumChunksForLength OK?

[Robert Sesek, 2017/02/27 23:15:28]

Hm, it should be OK. What keys are going to be whitelisted? Are they going to
get chunked, or can this be avoided totally?

[gsennton, 2017/02/28 21:02:42]

Removed these - with a more hard-coded (ugh!) version these aren't needed.

+size_t NumChunksForLength(size_t length, size_t chunk_max_length) {
+  // Compute (length / g_chunk_max_length_), rounded up.
+  return (length + chunk_max_length - 1) / chunk_max_length;
+}
+
+// TODO also copy-pasted from base/debug/crash_logging.cc
+const char kChunkFormatString[] = "%s-%" PRIuS;

[gsennton, 2017/02/27 18:31:38]

^^ same question here, should we BASE_EXPORT this?

+
+void SetupCrashKeysWhiteList(const std::vector<base::debug::CrashKey>& keys,
+                             const size_t chunk_max_length) {
+  LOG(ERROR) << "in SetupCrashKeysWhiteList";
+  // TODO do we care that we are storing lots of keys in our map?

[gsennton, 2017/02/27 18:31:38]

In our whitelist set we are storing an entry for each possible chunk, this can
be up to 16 entries per kLargeSize-key... is this OK or should I parse the
integer part of the key-value in IsInWhiteList instead? (do we have any utility
methods to parse integers from strings, without failing badly if there is no
number there?)

[Robert Sesek, 2017/02/27 23:15:28]

I think it's fine to just have up to 16 entries. How many large keys are you
going to store?

[gsennton, 2017/02/28 21:02:42]

+1, no large keys.

[gsennton, 2017/02/28 21:02:42]

Yeah, we only whitelist small/medium-sized keys, so this won't be an issue (for
now).

+  for (size_t key_index = 0; key_index < keys.size(); ++key_index) {
+    const size_t num_chunks =
+        NumChunksForLength(keys[key_index].max_length, chunk_max_length);
+    if (num_chunks == 1) {
+      g_crash_keys_whitelist.insert(keys[key_index].key_name);
+    } else {
+      for (size_t chunk_index = 0; chunk_index < num_chunks; ++chunk_index) {
+        g_crash_keys_whitelist.insert(base::StringPrintf(
+            kChunkFormatString, keys[key_index].key_name, chunk_index + 1));
+      }
+    }
+  }
+  LOG(ERROR) << "in SetupCrashKeysWhiteList, whitelist length: "
+             << g_crash_keys_whitelist.size();
+  for (auto it = g_crash_keys_whitelist.begin();
+       it != g_crash_keys_whitelist.end(); it++) {
+    LOG(ERROR) << "in SetupCrashKeysWhiteList, entry: " << *it;
+  }
+}
+
+bool IsInWhiteList(const base::StringPiece& key) {
+  // TODO does this need to be thread-safe?

[gsennton, 2017/02/27 18:31:38]

I'm guessing we allow SetCrashKeyValue to be called from different threads?

[Robert Sesek, 2017/02/27 23:15:28]

Yup. I'd ensure the whitelist is immutable after it's set.

[gsennton, 2017/02/28 21:02:42]

Done (well, I don't touch it again after the initial set-up).

+  return g_crash_keys_whitelist.find(key.as_string()) !=
+         g_crash_keys_whitelist.end();
+}
+
 void SetCrashKeyValue(const base::StringPiece& key,
                       const base::StringPiece& value) {
-  g_crash_keys->SetKeyValue(key.data(), value.data());
+  LOG(ERROR) << "in SetCrashKeyValue for key " << key.data();
+  // TODO if #DEFINED(USE_WHITELIST) --use-whitelist
+  if (IsInWhiteList(key)) {
+    LOG(ERROR) << "in SetCrashKeyValue for key " << key.data()
+               << " it is in the whitelist :D";
+    g_crash_keys->SetKeyValue(key.data(), value.data());
+  } else {
+    LOG(ERROR) << "in SetCrashKeyValue for key " << key.data()
+               << ", not in the whitelist";
+  }
 }
 
 void ClearCrashKey(const base::StringPiece& key) {
@@ -1112,6 +1166,9 @@ void ClearCrashKey(const base::StringPiece& key) {
 void InitCrashKeys() {
   g_crash_keys = new CrashKeyStorage;
   GetCrashReporterClient()->RegisterCrashKeys();
+  // TODO if #DEFINED(USE_WHITELIST) --use-whitelist
+  SetupCrashKeysWhiteList(GetCrashReporterClient()->GetWhiteListedCrashKeys(),
+                          crash_keys::kChunkMaxLength);
   base::debug::SetCrashKeyReportingFunctions(&SetCrashKeyValue, &ClearCrashKey);
 }