Prevent calling internal metrics code with invalid values.

chrome/browser/extensions/extension_metrics_module.cc

Index: chrome/browser/extensions/extension_metrics_module.cc
diff --git a/chrome/browser/extensions/extension_metrics_module.cc b/chrome/browser/extensions/extension_metrics_module.cc
index 48e5d0821093446bf1b9ea2f863f0f661f549ebf..b68786f8244f8b6a3c50d0c74ff11d001e921258 100644
--- a/chrome/browser/extensions/extension_metrics_module.cc
+++ b/chrome/browser/extensions/extension_metrics_module.cc
@@ -33,6 +33,15 @@ bool MetricsHistogramHelperFunction::RecordValue(const std::string& name,
                                                  int max,
                                                  size_t buckets,
                                                  int sample) {
+  // Make sure toxic values don't get to internal code

[jar (doing other things), 2012/01/05 17:35:14]

You should also watch out for INT_MAX since you're doing data checking.  We
should be far away, so that things like adding one or two don't push us over the
edge and wrap.

To avoid all sorts of overflows in malloc, you may as well bound the buckets at
"something reasonable."  I'd suggest nothing more than 10,000.  There is a
significant cost to such large histograms, and I'd rather just say "no" and use
a smaller bucket count than requested.  I think the underlying code may already
do something like this.

[rkc, 2012/01/06 00:08:01]

Done.

+  min = min > 0 ? min : 1;

[jar (doing other things), 2012/01/05 17:35:14]

nit: (personal style): I find it much more readable to use an if statement when
you optionally change a value, than to use a re-assign (as above) conditionally.
Suggest:
if (min < 1)
  min = 1;

Similar for line 38.

[rkc, 2012/01/06 00:08:01]

Done.

+  max = max > min ? max : min + 1;
+  buckets = buckets > 2 ? buckets : 3;
+  // Trim buckets down to a maximum of the given range + over/underflow buckets
+  if ((int)buckets > (max - min + 2)) {

[jar (doing other things), 2012/01/05 17:35:14]

nit: Use C++ style casts:  static_cast<int>(buckets)

If you bound buckets (as mentioned above) by something like 10,000, then you'll
be safe about overflows here as well during the cast.

[rkc, 2012/01/06 00:08:01]

Done.

+    buckets = max - min + 2;
+  }
+
   Histogram* counter;
   if (type == Histogram::LINEAR_HISTOGRAM) {
     counter = LinearHistogram::FactoryGet(name,