[SyzyAsan] Enable Crashpad reporter as a 50/50 experiment.

syzygy/agent/asan/runtime.cc

Index: syzygy/agent/asan/runtime.cc
diff --git a/syzygy/agent/asan/runtime.cc b/syzygy/agent/asan/runtime.cc
index bd851fb2179d9d31842e50a5dc89d1da12678361..e8395996a108b8cddd97083efc99d86bcd80d0c1 100644
--- a/syzygy/agent/asan/runtime.cc
+++ b/syzygy/agent/asan/runtime.cc
@@ -387,37 +387,44 @@ void LaunchMessageBox(const base::StringPiece& message) {
   ::MessageBoxA(nullptr, message.data(), nullptr, MB_OK | MB_ICONEXCLAMATION);
 }
 
-std::unique_ptr<ReporterInterface> CreateCrashReporter(AsanLogger* logger) {
+// Creates a crash reporter from the environment, if such an override is
+// configured.
+std::unique_ptr<ReporterInterface> CreateCrashReporterFromEnvironment(

[Sigurður Ásgeirsson, 2016/05/19 20:25:58]

MaybeCreate?

[chrisha, 2016/05/31 20:45:15]

Acknowledged.

+    AsanLogger* logger) {
+  DCHECK_NE(static_cast<AsanLogger*>(nullptr), logger);
   std::unique_ptr<ReporterInterface> reporter;
 
-  // First try to grab the preferred crash reporter, overridden by the
-  // environment.
   std::unique_ptr<base::Environment> env(base::Environment::Create());
   std::string reporter_name;
-  if (env->GetVar("SYZYASAN_CRASH_REPORTER", &reporter_name)) {
-    if (reporter_name == "crashpad")
-      reporter.reset(reporters::CrashpadReporter::Create().release());
-    else if (reporter_name == "kasko")
-      reporter.reset(reporters::KaskoReporter::Create().release());
-    else if (reporter_name == "breakpad")
-      reporter.reset(reporters::BreakpadReporter::Create().release());
-
-    if (reporter.get() != nullptr) {
-      logger->Write(base::StringPrintf(
-          "Using requested \"%s\" crash reporter.", reporter_name));
-      return reporter;
-    } else {
-      logger->Write(base::StringPrintf(
-          "Unable to create requested \"%s\" crash reporter.", reporter_name));
-    }
+  if (!env->GetVar("SYZYASAN_CRASH_REPORTER", &reporter_name))
+    return reporter;
+
+  if (reporter_name == "crashpad") {
+    reporter.reset(reporters::CrashpadReporter::Create().release());
+  } else if (reporter_name == "kasko") {
+    reporter.reset(reporters::KaskoReporter::Create().release());
+  } else if (reporter_name == "breakpad") {
+    reporter.reset(reporters::BreakpadReporter::Create().release());
+  }
+
+  if (reporter.get() != nullptr) {
+    logger->Write(base::StringPrintf(
+        "Using requested \"%s\" crash reporter.", reporter_name.c_str()));
+    return reporter;
+  } else {
+    logger->Write(base::StringPrintf(
+        "Unable to create requested \"%s\" crash reporter.",
+        reporter_name.c_str()));
   }
 
-  // No crash reporter was explicitly specified, or it was unable to be
-  // initialized. Try all of them, starting with the most recent.
+  return reporter;
+}
+
+// Attempts to create a crash reporter, starting with the most modern.
+std::unique_ptr<ReporterInterface> CreateCrashReporter(AsanLogger* logger) {
+  std::unique_ptr<ReporterInterface> reporter;
 
-  // Don't try grabbing a Crashpad reporter by default, as we're not yet
-  // ready to ship this.
-  // TODO(chrisha): Add Crashpad support behind a feature flag.
+  // Crashpad integration is not yet enabled by default.
 
   // Try to initialize a Kasko crash reporter.
   if (reporter.get() == nullptr)

[Sigurður Ásgeirsson, 2016/05/19 20:25:58]

this is a noop

[chrisha, 2016/05/31 20:45:15]

Acknowledged.

@@ -483,21 +490,24 @@ bool AsanRuntime::SetUp(const std::wstring& flags_command_line) {
     return false;
   WindowsHeapAdapter::SetUp(heap_manager_.get());
 
+  // If the environment overrides randomized features or parameters then use
+  // it to initialize a crash reporter.
+  crash_reporter_.reset(
+      CreateCrashReporterFromEnvironment(logger()).release());
+
   if (params_.feature_randomization) {
     AsanFeatureSet feature_set = GenerateRandomFeatureSet();
+    // If none is yet created, this call may potentially create a crash
+    // handler.
     PropagateFeatureSet(feature_set);

[Sigurður Ásgeirsson, 2016/05/19 20:25:58]

I don't follow the logic in this file at all - there seems to be a scattering of
policy decision all over the place, and then piecewise acting on it.
Will this e.g. report the CrashPad experiment if parameters specify
randomization and the environment forces Kasko?

Maybe it makes more sense to consolidate the policy decision somewhere, and to
report the crash reporter used as the group?

[chrisha, 2016/05/19 20:31:31]

I agree, it's scattered and messy. I really don't want to have to refactor this,
and would much rather Seb own that.

The reporting is already separated from all of this code, and the actual crash
reporter in use is the one that will be reported. See 'GetEnabledFeatureSet'
below.

[Sigurður Ásgeirsson, 2016/05/20 14:00:17]

Filed https://github.com/google/syzygy/issues/45 and assigned to Seb.
That's returning a binary value for a trinary decision :(.

   }
 
-  // Propagates the flags values to the different modules.
+  // Propagates the flags values to the different modules. If none is yet
+  // created, this call may potentially create a crash handler.
   PropagateParams();
 
-  // The name 'disable_breakpad_reporting' is legacy; this actually means to
-  // disable all external crash reporting integration.
-  if (!params_.disable_breakpad_reporting)
-    crash_reporter_.reset(CreateCrashReporter(logger()).release());
-
   // Set up the appropriate error handler depending on whether or not
-  // we successfully found a crash reporter.
+  // we successfully initialized a crash reporter.
   if (crash_reporter_.get() != nullptr) {
     logger_->Write(base::StringPrintf(
         "SyzyASAN: Using %s for error reporting.",
@@ -814,6 +824,11 @@ void AsanRuntime::PropagateParams() {
   logger_->set_log_as_text(params_.log_as_text);
   // exit_on_failure is used locally by AsanRuntime.
   logger_->set_minidump_on_failure(params_.minidump_on_failure);
+
+  // The name 'disable_breakpad_reporting' is legacy; this actually means to
+  // disable all external crash reporting integration.
+  if (!params_.disable_breakpad_reporting && crash_reporter_.get() == nullptr)
+    crash_reporter_.reset(CreateCrashReporter(logger()).release());
 }
 
 size_t AsanRuntime::CalculateCorruptHeapInfoSize(
@@ -961,6 +976,7 @@ void AsanRuntime::LogAsanErrorInfo(AsanErrorInfo* error_info) {
   }
 }
 
+// static
 AsanFeatureSet AsanRuntime::GenerateRandomFeatureSet() {
   AsanFeatureSet enabled_features =
       static_cast<AsanFeatureSet>(base::RandGenerator(ASAN_FEATURE_MAX));
@@ -975,6 +991,10 @@ void AsanRuntime::PropagateFeatureSet(AsanFeatureSet feature_set) {
       (feature_set & ASAN_FEATURE_ENABLE_PAGE_PROTECTIONS) != 0;
   params_.enable_large_block_heap =
       (feature_set & ASAN_FEATURE_ENABLE_LARGE_BLOCK_HEAP) != 0;
+  if ((feature_set & ASAN_FEATURE_ENABLE_CRASHPAD) != 0 &&
+      crash_reporter_.get() == nullptr) {
+    crash_reporter_.reset(reporters::CrashpadReporter::Create().release());
+  }
 }
 
 void AsanRuntime::GetBadAccessInformation(AsanErrorInfo* error_info) {
@@ -1199,6 +1219,8 @@ AsanFeatureSet AsanRuntime::GetEnabledFeatureSet() {
     enabled_features |= ASAN_FEATURE_ENABLE_PAGE_PROTECTIONS;
   if (params_.enable_large_block_heap)
     enabled_features |= ASAN_FEATURE_ENABLE_LARGE_BLOCK_HEAP;
+  if (crash_reporter_->GetName() == reporters::CrashpadReporter::kName)
+    enabled_features |= ASAN_FEATURE_ENABLE_CRASHPAD;

[Sigurður Ásgeirsson, 2016/05/20 14:00:17]

Given I can't follow the code above, I'm not convinced that crash_reporter_
can't be nullptr?
Also, we really have three groups - Kasko, Breakpad, CrashPad?

[chrisha, 2016/05/31 20:45:15]

Breakpad can't really happen unless manually overridden, as Kasko is always
present. I could add it, but it felt like overkill. I'd have to rework the
entire experiment exporting logic, which can only handle true false.

Other than via a very specific bit of manual intervention, the crash reporter
can only be one of Crashpad or Kasko.

 
   return enabled_features;
 }