Prevent calling internal metrics code with invalid values.

Prevent calling internal metrics code with invalid values.
Currently we can call the internal metrics code in Chrome from the metrics extension with completely arbitrary values; some of which can cause the creation of invalid histograms which in turn can Chrome to crash. This CL sanitizes the inputs to the extension so that the histogram constructed is valid.

Currently this is causing one crash due to code from http://codereview.chromium.org/8819013
This CL fixes the issue with the extension and fixes the crash. The issues with the JS code still needs to be fixed in another CL.

R=jar@chromium.org
BUG=chromium-os:24115
TEST=Tried the repro case for the crash several times to confirm that it isn't happening anymore.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116627

[jar (doing other things), 2012-01-05 17:35:13]

http://codereview.chromium.org/9113002/diff/1/base/metrics/histogram.h
File base/metrics/histogram.h (right):

http://codereview.chromium.org/9113002/diff/1/base/metrics/histogram.h#newcod...
base/metrics/histogram.h:434: // minimum > 0
There is fixup code in histograms for this, so minimum can be smaller... but it
is implicitly fixed up to 1. I'd suggest changing line 434 to read:

// if minimum is not positive, it is implicitly replaced by 1.

http://codereview.chromium.org/9113002/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_metrics_module.cc (right):

http://codereview.chromium.org/9113002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_metrics_module.cc:36: // Make sure toxic
values don't get to internal code
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.

http://codereview.chromium.org/9113002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_metrics_module.cc:37: min = min > 0 ? min :
1;
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.

http://codereview.chromium.org/9113002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_metrics_module.cc:41: if ((int)buckets >
(max - min + 2)) {
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]

Review changes incorporated.

http://codereview.chromium.org/9113002/diff/1/base/metrics/histogram.h
File base/metrics/histogram.h (right):

http://codereview.chromium.org/9113002/diff/1/base/metrics/histogram.h#newcod...
base/metrics/histogram.h:434: // minimum > 0
On 2012/01/05 17:35:14, jar wrote:
> There is fixup code in histograms for this, so minimum can be smaller... but
it
> is implicitly fixed up to 1. I'd suggest changing line 434 to read:
> 
> // if minimum is not positive, it is implicitly replaced by 1.

Done.

http://codereview.chromium.org/9113002/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_metrics_module.cc (right):

http://codereview.chromium.org/9113002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_metrics_module.cc:36: // Make sure toxic
values don't get to internal code
On 2012/01/05 17:35:14, jar wrote:
> 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.

Done.

http://codereview.chromium.org/9113002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_metrics_module.cc:37: min = min > 0 ? min :
1;
On 2012/01/05 17:35:14, jar wrote:
> 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.

Done.

http://codereview.chromium.org/9113002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_metrics_module.cc:41: if ((int)buckets >
(max - min + 2)) {
On 2012/01/05 17:35:14, jar wrote:
> 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.

Done.

[jar (doing other things), 2012-01-06 02:00:10]

http://codereview.chromium.org/9113002/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_metrics_module.cc (right):

http://codereview.chromium.org/9113002/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_metrics_module.cc:49: max = std::min(max,
INT_MAX - 3);
You need to do these before line 44 and 45.

[rkc, 2012-01-06 02:08:25]

Review changes incorporated.

http://codereview.chromium.org/9113002/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_metrics_module.cc (right):

http://codereview.chromium.org/9113002/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_metrics_module.cc:49: max = std::min(max,
INT_MAX - 3);
On 2012/01/06 02:00:11, jar wrote:
> You need to do these before line 44 and 45.

Done.

[jar (doing other things), 2012-01-06 02:11:32]

lgtm