Prevent calling internal metrics code with invalid values.

chrome/browser/extensions/extension_metrics_module.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_metrics_module.h"
 
 #include "base/metrics/histogram.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/user_metrics.h"
 
@@ skipping 15 matching lines @@
   EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(1, sample));
   return true;
 }
 
 bool MetricsHistogramHelperFunction::RecordValue(const std::string& name,
                                                  Histogram::ClassType type,
                                                  int min,
                                                  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,
                                           min,
                                           max,
                                           buckets,
                                           Histogram::kUmaTargetedHistogramFlag);
   } else {
     counter = Histogram::FactoryGet(name,
                                     min,
@@ skipping 74 matching lines @@
   return RecordValue(name, Histogram::HISTOGRAM, 1, kThreeMinMs, 50, sample);
 }
 
 bool MetricsRecordLongTimeFunction::RunImpl() {
   std::string name;
   int sample;
   EXTENSION_FUNCTION_VALIDATE(GetNameAndSample(&name, &sample));
   static const int kOneHourMs = 60 * 60 * 1000;
   return RecordValue(name, Histogram::HISTOGRAM, 1, kOneHourMs, 50, sample);
 }