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

syzygy/agent/asan/runtime.cc

 // Copyright 2012 Google Inc. All Rights Reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //     http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
@@ skipping 369 matching lines @@
           ->WriteCorruptHeapInfo(corrupt_ranges, size, buffer, error_info); \
     }                                                                       \
   }
 
 void LaunchMessageBox(const base::StringPiece& message) {
   // TODO(chrisha): Consider making this close itself with a timeout to prevent
   //     hangs on the waterfall.
   ::MessageBoxA(nullptr, message.data(), nullptr, MB_OK | MB_ICONEXCLAMATION);
 }
 
+// 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;
+
+  std::unique_ptr<base::Environment> env(base::Environment::Create());
+  std::string 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()));
+  }
+
+  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;
 
-  // 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));
-    }
-  }
-
-  // No crash reporter was explicitly specified, or it was unable to be
-  // initialized. Try all of them, starting with the most recent.
-
-  // 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.

     reporter.reset(reporters::KaskoReporter::Create().release());
 
   // If that failed then try to initialize a Breakpad reporter.
   if (reporter.get() == nullptr)
     reporter.reset(reporters::BreakpadReporter::Create().release());
 
   return reporter;
 }
 
 }  // namespace
@@ skipping 42 matching lines @@
   if (!SetUpMemoryNotifier())
     return false;
   if (!SetUpLogger())
     return false;
   if (!SetUpStackCache())
     return false;
   if (!SetUpHeapManager())
     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.",
         crash_reporter_->GetName()));
     SetErrorCallBack(base::Bind(&CrashReporterErrorHandler));
   } else {
     logger_->Write("SyzyASAN: Using default error reporting handler.");
     SetErrorCallBack(base::Bind(&DefaultErrorHandler));
   }
 
@@ skipping 296 matching lines @@
   // Push the configured parameter values to the appropriate endpoints.
   heap_manager_->set_parameters(params_);
   StackCaptureCache::set_compression_reporting_period(params_.reporting_period);
   common::StackCapture::set_bottom_frames_to_skip(
       params_.bottom_frames_to_skip);
   stack_cache_->set_max_num_frames(params_.max_num_frames);
   // ignored_stack_ids is used locally by AsanRuntime.
   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(
     const HeapChecker::CorruptRangesVector& corrupt_ranges) {
   size_t n = corrupt_ranges.size() *
              (sizeof(AsanCorruptBlockRange) + sizeof(AsanBlockInfo));
   return n;
 }
 
 void AsanRuntime::WriteCorruptHeapInfo(
@@ skipping 127 matching lines @@
   }
 
   // Print the Windbg information to display the free stack if present.
   if (error_info->block_info.free_stack_size != NULL) {
     AsanDbgMessage(L"Free stack trace:");
     AsanDbgCmd(L"dps %p l%d", error_info->block_info.free_stack,
                error_info->block_info.free_stack_size);
   }
 }
 
+// static
 AsanFeatureSet AsanRuntime::GenerateRandomFeatureSet() {
   AsanFeatureSet enabled_features =
       static_cast<AsanFeatureSet>(base::RandGenerator(ASAN_FEATURE_MAX));
   DCHECK_LT(enabled_features, ASAN_FEATURE_MAX);
   enabled_features &= kAsanValidFeatures;
   return enabled_features;
 }
 
 void AsanRuntime::PropagateFeatureSet(AsanFeatureSet feature_set) {
   DCHECK_EQ(0U, feature_set & ~kAsanValidFeatures);
   heap_manager_->enable_page_protections_ =
       (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) {
   base::AutoLock lock(lock_);
 
   // Checks if this is an access to an internal structure or if it's an access
   // in the upper region of the memory (over the 2 GB limit).
   if ((reinterpret_cast<size_t>(error_info->location) & (1 << 31)) != 0 ||
       shadow()->GetShadowMarkerForAddress(error_info->location) ==
           kAsanMemoryMarker) {
@@ skipping 204 matching lines @@
   DCHECK(heap_manager_);
   heap_manager_->DisableDeferredFreeThread();
 }
 
 AsanFeatureSet AsanRuntime::GetEnabledFeatureSet() {
   AsanFeatureSet enabled_features = static_cast<AsanFeatureSet>(0U);
   if (heap_manager_->enable_page_protections_)
     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;
 }
 
 }  // namespace asan
 }  // namespace agent