Fix InspectConstructionArguments with random min & max.

If the min/max parameters are really fubar, such as which might come
from a malicious IPC message, min & max can be limited only to then
be swapped and become invalid.  Do the swap first.

Also, remove duplicate bucket-limit condition and set the defined
maximum to be the lower of the two.

Bug: 905226
Change-Id: If73e97e43e93704f93456da6af0708422fa145a0
Reviewed-on: https://chromium-review.googlesource.com/c/1335731
Commit-Queue: Brian White <bcwhite@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608398}

base/metrics/histogram.cc

@@ -93,7 +93,7 @@ typedef HistogramBase::Count Count;
 typedef HistogramBase::Sample Sample;
 
 // static
-const uint32_t Histogram::kBucketCount_MAX = 16384u;
+const uint32_t Histogram::kBucketCount_MAX = 10002u;
 
 class Histogram::Factory {
  public:
@@ -523,27 +523,37 @@ bool Histogram::InspectConstructionArguments(StringPiece name,
                                              Sample* minimum,
                                              Sample* maximum,
                                              uint32_t* bucket_count) {
+  bool check_okay = true;
+
+  // Checks below must be done after any min/max swap.
+  if (*minimum > *maximum) {
+    check_okay = false;
+    std::swap(*minimum, *maximum);
+  }
+
   // Defensive code for backward compatibility.
   if (*minimum < 1) {
     DVLOG(1) << "Histogram: " << name << " has bad minimum: " << *minimum;
     *minimum = 1;
+    if (*maximum < 1)
+      *maximum = 1;
   }
   if (*maximum >= kSampleType_MAX) {
     DVLOG(1) << "Histogram: " << name << " has bad maximum: " << *maximum;
     *maximum = kSampleType_MAX - 1;
   }
   if (*bucket_count >= kBucketCount_MAX) {
+    check_okay = false;
     DVLOG(1) << "Histogram: " << name << " has bad bucket_count: "
              << *bucket_count;
     *bucket_count = kBucketCount_MAX - 1;
   }
-
-  bool check_okay = true;
-
-  if (*minimum > *maximum) {
-    check_okay = false;
-    std::swap(*minimum, *maximum);
+  if (*bucket_count > 1002) {
+    UmaHistogramSparse("Histogram.TooManyBuckets.1000",
+                       static_cast<Sample>(HashMetricName(name)));
   }
+
+  // Ensure parameters are sane.
   if (*maximum == *minimum) {
     check_okay = false;
     *maximum = *minimum + 1;
@@ -552,13 +562,6 @@ bool Histogram::InspectConstructionArguments(StringPiece name,
     check_okay = false;
     *bucket_count = 3;
   }
-  // Very high bucket counts are wasteful. Use a sparse histogram instead.
-  // Value of 10002 equals a user-supplied value of 10k + 2 overflow buckets.
-  constexpr uint32_t kMaxBucketCount = 10002;
-  if (*bucket_count > kMaxBucketCount) {
-    check_okay = false;
-    *bucket_count = kMaxBucketCount;
-  }
   if (*bucket_count > static_cast<uint32_t>(*maximum - *minimum + 2)) {
     check_okay = false;
     *bucket_count = static_cast<uint32_t>(*maximum - *minimum + 2);

tools/metrics/histograms/histograms.xml

@@ -37820,6 +37820,17 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="Histogram.TooManyBuckets.1000" enum="HistogramNameHash"
+    expires_after="M73">
+  <owner>asvitkine@chromium.org</owner>
+  <owner>bcwhite@chromium.org</owner>
+  <summary>
+    The hash codes of histograms that were found to request more than 1000
+    buckets. These would be DCHECK exceptions in debug builds if the limit is
+    lowered so are being logged before that change.
+  </summary>
+</histogram>
+
 <histogram name="History.AttemptedToFixProfileError">
   <owner>brettw@chromium.org</owner>
   <summary>