GWP-ASan: Refactor process sampling boost

Previously the caller to gwp_asan::EnableFor* would pass in various
bools to indicate if sampling should be increased because this is an
interesting process/release channel. This doesn't work well in a world
where GWP-ASan hooks multiple allocators, instead have the caller simply
indicate that process sampling should be boosted or not--not the
specific conditions that led to increased process sampling.

In doing so also change the previous ProcessSamplingBoost feature
name/semantics to ProcessSamplingBoost2 since the boost is no longer
cumulative.

Bug: 956824
Change-Id: I70f646b15df5f002857c08bb104ffd881b5f2f3a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1626031
Commit-Queue: Vlad Tsyrklevich <vtsyrklevich@chromium.org>
Reviewed-by: Vitaly Buka <vitalybuka@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Auto-Submit: Vlad Tsyrklevich <vtsyrklevich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662727}

chrome/app/chrome_main_delegate.cc

@@ -569,7 +569,7 @@ void ChromeMainDelegate::PostFieldTrialInitialization() {
     std::string process_type =
         command_line.GetSwitchValueASCII(switches::kProcessType);
     bool is_browser_process = process_type.empty();
-    gwp_asan::EnableForMalloc(is_canary_dev, is_browser_process);
+    gwp_asan::EnableForMalloc(is_canary_dev || is_browser_process);
   }
 #endif
 

components/gwp_asan/client/gwp_asan.cc

@@ -54,10 +54,8 @@ constexpr int kDefaultAllocationSamplingRange = 64;
 
 constexpr double kDefaultProcessSamplingProbability = 0.2;
 // The multiplier to increase the ProcessSamplingProbability in scenarios where
-// we want to perform additional testing (e.g. on canary/dev builds or in the
-// browser process.) The multiplier increase is cumulative when multiple
-// conditions apply.
-constexpr int kDefaultProcessSamplingBoost = 4;
+// we want to perform additional testing (e.g., on canary/dev builds).
+constexpr int kDefaultProcessSamplingBoost2 = 5;
 
 const base::Feature kGwpAsanMalloc{"GwpAsanMalloc",
                                    base::FEATURE_ENABLED_BY_DEFAULT};
@@ -65,9 +63,7 @@ const base::Feature kGwpAsanPartitionAlloc{"GwpAsanPartitionAlloc",
                                            base::FEATURE_DISABLED_BY_DEFAULT};
 
 // Returns whether this process should be sampled to enable GWP-ASan.
-bool SampleProcess(const base::Feature& feature,
-                   bool is_canary_dev,
-                   bool is_browser_process) {
+bool SampleProcess(const base::Feature& feature, bool boost_sampling) {
   double process_sampling_probability =
       GetFieldTrialParamByFeatureAsDouble(feature, "ProcessSamplingProbability",
                                           kDefaultProcessSamplingProbability);
@@ -79,24 +75,20 @@ bool SampleProcess(const base::Feature& feature,
   }
 
   int process_sampling_boost = GetFieldTrialParamByFeatureAsInt(
-      feature, "ProcessSamplingBoost", kDefaultProcessSamplingBoost);
-  base::CheckedNumeric<int> multiplier = 1;
-  if (is_canary_dev)
-    multiplier += process_sampling_boost;
-  if (is_browser_process)
-    multiplier += process_sampling_boost;
-
-  if (!multiplier.IsValid() || multiplier.ValueOrDie() < 1) {
-    DLOG(ERROR) << "GWP-ASan ProcessSampling multiplier is out-of-range";
+      feature, "ProcessSamplingBoost2", kDefaultProcessSamplingBoost2);
+  if (process_sampling_boost < 1) {
+    DLOG(ERROR) << "GWP-ASan ProcessSampling multiplier is out-of-range: "
+                << process_sampling_boost;
     return false;
   }
 
   base::CheckedNumeric<double> sampling_prob_mult =
       process_sampling_probability;
-  sampling_prob_mult *= multiplier.ValueOrDie();
+  if (boost_sampling)
+    sampling_prob_mult *= process_sampling_boost;
   if (!sampling_prob_mult.IsValid()) {
     DLOG(ERROR) << "GWP-ASan multiplier caused out-of-range multiply: "
-                << multiplier.ValueOrDie();
+                << process_sampling_boost;
     return false;
   }
 
@@ -138,8 +130,7 @@ size_t AllocationSamplingFrequency(const base::Feature& feature) {
 // Exported for testing.
 GWP_ASAN_EXPORT base::Optional<AllocatorSettings> GetAllocatorSettings(
     const base::Feature& feature,
-    bool is_canary_dev,
-    bool is_browser_process) {
+    bool boost_sampling) {
   if (!base::FeatureList::IsEnabled(feature))
     return base::nullopt;
 
@@ -178,7 +169,7 @@ GWP_ASAN_EXPORT base::Optional<AllocatorSettings> GetAllocatorSettings(
   if (!alloc_sampling_freq)
     return base::nullopt;
 
-  if (!SampleProcess(feature, is_canary_dev, is_browser_process))
+  if (!SampleProcess(feature, boost_sampling))
     return base::nullopt;
 
   return AllocatorSettings{max_allocations, max_metadata, total_pages,
@@ -187,11 +178,11 @@ GWP_ASAN_EXPORT base::Optional<AllocatorSettings> GetAllocatorSettings(
 
 }  // namespace internal
 
-void EnableForMalloc(bool is_canary_dev, bool is_browser_process) {
+void EnableForMalloc(bool boost_sampling) {
 #if BUILDFLAG(USE_ALLOCATOR_SHIM)
   static bool init_once = [&]() -> bool {
-    auto settings = internal::GetAllocatorSettings(
-        internal::kGwpAsanMalloc, is_canary_dev, is_browser_process);
+    auto settings = internal::GetAllocatorSettings(internal::kGwpAsanMalloc,
+                                                   boost_sampling);
     if (!settings)
       return false;
 
@@ -207,11 +198,11 @@ void EnableForMalloc(bool is_canary_dev, bool is_browser_process) {
 #endif  // BUILDFLAG(USE_ALLOCATOR_SHIM)
 }
 
-void EnableForPartitionAlloc(bool is_canary_dev) {
+void EnableForPartitionAlloc(bool boost_sampling) {
 #if BUILDFLAG(USE_PARTITION_ALLOC)
   static bool init_once = [&]() -> bool {
     auto settings = internal::GetAllocatorSettings(
-        internal::kGwpAsanPartitionAlloc, is_canary_dev, false);
+        internal::kGwpAsanPartitionAlloc, boost_sampling);
     if (!settings)
       return false;
 

components/gwp_asan/client/gwp_asan.h

@@ -23,11 +23,11 @@ struct AllocatorSettings {
 
 // Enable GWP-ASan for the current process for the given allocator. This should
 // only be called once per process. This can not be disabled once it has been
-// enabled.
+// enabled. The |boost_sampling| parameter is used to indicate if the caller
+// wants to increase the sampling for this process.
 
-GWP_ASAN_EXPORT void EnableForMalloc(bool is_canary_dev,
-                                     bool is_browser_process);
-GWP_ASAN_EXPORT void EnableForPartitionAlloc(bool is_canary_dev);
+GWP_ASAN_EXPORT void EnableForMalloc(bool boost_sampling);
+GWP_ASAN_EXPORT void EnableForPartitionAlloc(bool boost_sampling);
 
 }  // namespace gwp_asan
 

components/gwp_asan/client/gwp_asan_unittest.cc

@@ -22,8 +22,7 @@ namespace internal {
 
 base::Optional<AllocatorSettings> GetAllocatorSettings(
     const base::Feature& feature,
-    bool is_canary_dev,
-    bool is_browser_process);
+    bool boost_sampling);
 
 namespace {
 
@@ -40,15 +39,14 @@ size_t processSamplingTest(const char* process_sampling,
   std::map<std::string, std::string> parameters;
   parameters["ProcessSamplingProbability"] = process_sampling;
   if (process_sampling_boost)
-    parameters["ProcessSamplingBoost"] = process_sampling_boost;
+    parameters["ProcessSamplingBoost2"] = process_sampling_boost;
 
   base::test::ScopedFeatureList scoped_feature;
   scoped_feature.InitAndEnableFeatureWithParameters(kTestFeature1, parameters);
 
   size_t enabled = 0;
   for (size_t i = 0; i < kLoopIterations; i++) {
-    if (GetAllocatorSettings(kTestFeature1, process_sampling_boost != nullptr,
-                             false))
+    if (GetAllocatorSettings(kTestFeature1, process_sampling_boost != nullptr))
       enabled++;
   }
 
@@ -71,7 +69,7 @@ std::set<size_t> allocationSamplingTest(
 
   std::set<size_t> frequencies;
   for (size_t i = 0; i < kLoopIterations; i++) {
-    if (auto settings = GetAllocatorSettings(kTestFeature2, false, false))
+    if (auto settings = GetAllocatorSettings(kTestFeature2, false))
       frequencies.insert(settings->sampling_frequency);
   }
 
@@ -82,15 +80,18 @@ std::set<size_t> allocationSamplingTest(
 
 TEST(GwpAsanTest, ProcessSamplingWorks) {
   EXPECT_EQ(processSamplingTest("1.0", nullptr), kLoopIterations);
-  EXPECT_EQ(processSamplingTest("1.0", "99999"), kLoopIterations);
-  EXPECT_EQ(processSamplingTest("0.01", "99"), kLoopIterations);
+  EXPECT_EQ(processSamplingTest("1.0", "100000"), kLoopIterations);
+  EXPECT_EQ(processSamplingTest("0.01", "100"), kLoopIterations);
 
   EXPECT_EQ(processSamplingTest("0.0", nullptr), 0U);
-  EXPECT_EQ(processSamplingTest("0.0", "99999"), 0U);
+  EXPECT_EQ(processSamplingTest("0.0", "100000"), 0U);
 
   size_t num_enabled = processSamplingTest("0.5", nullptr);
   EXPECT_GT(num_enabled, 0U);
   EXPECT_LT(num_enabled, kLoopIterations);
+  num_enabled = processSamplingTest("0.01", "50");
+  EXPECT_GT(num_enabled, 0U);
+  EXPECT_LT(num_enabled, kLoopIterations);
 }
 
 TEST(GwpAsanTest, AllocationSamplingWorks) {