Fix overflow bug in histogram sample data structures
BUG=580696
BUG=internal b/26683692
TEST=Modified tests pass. End to end testing verified on problem scenario.
Review URL: https://codereview.chromium.org/1618423004
Cr-Commit-Position: refs/heads/master@{#371118}
diff --git a/base/metrics/histogram.h b/base/metrics/histogram.h
index 28bb29b6..93f704a 100644
--- a/base/metrics/histogram.h
+++ b/base/metrics/histogram.h
@@ -220,10 +220,7 @@
// Allow tests to corrupt our innards for testing purposes.
FRIEND_TEST_ALL_PREFIXES(HistogramTest, BoundsTest);
FRIEND_TEST_ALL_PREFIXES(HistogramTest, BucketPlacementTest);
- FRIEND_TEST_ALL_PREFIXES(HistogramTest, CorruptBucketBounds);
FRIEND_TEST_ALL_PREFIXES(HistogramTest, CorruptSampleCounts);
- FRIEND_TEST_ALL_PREFIXES(HistogramTest, NameMatchTest);
- FRIEND_TEST_ALL_PREFIXES(HistogramTest, AddCountTest);
friend class StatisticsRecorder; // To allow it to delete duplicates.
friend class StatisticsRecorderTest;
diff --git a/base/metrics/histogram_unittest.cc b/base/metrics/histogram_unittest.cc
index 2fadc30..f9302885 100644
--- a/base/metrics/histogram_unittest.cc
+++ b/base/metrics/histogram_unittest.cc
@@ -235,7 +235,6 @@
EXPECT_EQ(HistogramBase::kSampleType_MAX, ranges->range(2));
}
-// Test the AddCount function.
TEST_F(HistogramTest, AddCountTest) {
const size_t kBucketCount = 50;
Histogram* histogram = static_cast<Histogram*>(
@@ -245,7 +244,7 @@
histogram->AddCount(20, 15);
histogram->AddCount(30, 14);
- scoped_ptr<SampleVector> samples = histogram->SnapshotSampleVector();
+ scoped_ptr<HistogramSamples> samples = histogram->SnapshotSamples();
EXPECT_EQ(29, samples->TotalCount());
EXPECT_EQ(15, samples->GetCount(20));
EXPECT_EQ(14, samples->GetCount(30));
@@ -253,12 +252,36 @@
histogram->AddCount(20, 25);
histogram->AddCount(30, 24);
- scoped_ptr<SampleVector> samples2 = histogram->SnapshotSampleVector();
+ scoped_ptr<HistogramSamples> samples2 = histogram->SnapshotSamples();
EXPECT_EQ(78, samples2->TotalCount());
EXPECT_EQ(40, samples2->GetCount(20));
EXPECT_EQ(38, samples2->GetCount(30));
}
+TEST_F(HistogramTest, AddCount_LargeValuesDontOverflow) {
+ const size_t kBucketCount = 50;
+ Histogram* histogram = static_cast<Histogram*>(
+ Histogram::FactoryGet("AddCountHistogram", 10, 1000000000, kBucketCount,
+ HistogramBase::kNoFlags));
+
+ histogram->AddCount(200000000, 15);
+ histogram->AddCount(300000000, 14);
+
+ scoped_ptr<HistogramSamples> samples = histogram->SnapshotSamples();
+ EXPECT_EQ(29, samples->TotalCount());
+ EXPECT_EQ(15, samples->GetCount(200000000));
+ EXPECT_EQ(14, samples->GetCount(300000000));
+
+ histogram->AddCount(200000000, 25);
+ histogram->AddCount(300000000, 24);
+
+ scoped_ptr<HistogramSamples> samples2 = histogram->SnapshotSamples();
+ EXPECT_EQ(78, samples2->TotalCount());
+ EXPECT_EQ(40, samples2->GetCount(200000000));
+ EXPECT_EQ(38, samples2->GetCount(300000000));
+ EXPECT_EQ(19400000000LL, samples2->sum());
+}
+
// Make sure histogram handles out-of-bounds data gracefully.
TEST_F(HistogramTest, BoundsTest) {
const size_t kBucketCount = 50;
@@ -358,7 +381,7 @@
Histogram* histogram = static_cast<Histogram*>(
Histogram::FactoryGet("Histogram", 1, 64, 8, HistogramBase::kNoFlags));
- scoped_ptr<SampleVector> snapshot = histogram->SnapshotSampleVector();
+ scoped_ptr<HistogramSamples> snapshot = histogram->SnapshotSamples();
EXPECT_EQ(HistogramBase::NO_INCONSISTENCIES,
histogram->FindCorruption(*snapshot));
diff --git a/base/metrics/sample_map.cc b/base/metrics/sample_map.cc
index a691243..e276b91 100644
--- a/base/metrics/sample_map.cc
+++ b/base/metrics/sample_map.cc
@@ -19,7 +19,7 @@
void SampleMap::Accumulate(Sample value, Count count) {
sample_counts_[value] += count;
- IncreaseSum(count * value);
+ IncreaseSum(static_cast<int64_t>(count) * value);
IncreaseRedundantCount(count);
}
diff --git a/base/metrics/sample_map_unittest.cc b/base/metrics/sample_map_unittest.cc
index c941d65..3626bd0a 100644
--- a/base/metrics/sample_map_unittest.cc
+++ b/base/metrics/sample_map_unittest.cc
@@ -24,6 +24,20 @@
EXPECT_EQ(samples.redundant_count(), samples.TotalCount());
}
+TEST(SampleMapTest, Accumulate_LargeValuesDontOverflow) {
+ SampleMap samples(1);
+
+ samples.Accumulate(250000000, 100);
+ samples.Accumulate(500000000, 200);
+ samples.Accumulate(250000000, -200);
+ EXPECT_EQ(-100, samples.GetCount(250000000));
+ EXPECT_EQ(200, samples.GetCount(500000000));
+
+ EXPECT_EQ(75000000000LL, samples.sum());
+ EXPECT_EQ(100, samples.TotalCount());
+ EXPECT_EQ(samples.redundant_count(), samples.TotalCount());
+}
+
TEST(SampleMapTest, AddSubtractTest) {
SampleMap samples1(1);
SampleMap samples2(2);
diff --git a/base/metrics/sample_vector.cc b/base/metrics/sample_vector.cc
index 46faef0..ccb9431 100644
--- a/base/metrics/sample_vector.cc
+++ b/base/metrics/sample_vector.cc
@@ -43,7 +43,7 @@
size_t bucket_index = GetBucketIndex(value);
subtle::NoBarrier_Store(&counts_[bucket_index],
subtle::NoBarrier_Load(&counts_[bucket_index]) + count);
- IncreaseSum(count * value);
+ IncreaseSum(static_cast<int64_t>(count) * value);
IncreaseRedundantCount(count);
}
diff --git a/base/metrics/sample_vector_unittest.cc b/base/metrics/sample_vector_unittest.cc
index 744cbfab..434def78 100644
--- a/base/metrics/sample_vector_unittest.cc
+++ b/base/metrics/sample_vector_unittest.cc
@@ -44,6 +44,33 @@
EXPECT_EQ(samples.TotalCount(), samples.redundant_count());
}
+TEST(SampleVectorTest, Accumulate_LargeValuesDontOverflow) {
+ // Custom buckets: [1, 250000000) [250000000, 500000000)
+ BucketRanges ranges(3);
+ ranges.set_range(0, 1);
+ ranges.set_range(1, 250000000);
+ ranges.set_range(2, 500000000);
+ SampleVector samples(1, &ranges);
+
+ samples.Accumulate(240000000, 200);
+ samples.Accumulate(249999999, -300);
+ EXPECT_EQ(-100, samples.GetCountAtIndex(0));
+
+ samples.Accumulate(250000000, 200);
+ EXPECT_EQ(200, samples.GetCountAtIndex(1));
+
+ EXPECT_EQ(23000000300LL, samples.sum());
+ EXPECT_EQ(100, samples.redundant_count());
+ EXPECT_EQ(samples.TotalCount(), samples.redundant_count());
+
+ samples.Accumulate(250000000, -100);
+ EXPECT_EQ(100, samples.GetCountAtIndex(1));
+
+ EXPECT_EQ(-1999999700LL, samples.sum());
+ EXPECT_EQ(0, samples.redundant_count());
+ EXPECT_EQ(samples.TotalCount(), samples.redundant_count());
+}
+
TEST(SampleVectorTest, AddSubtractTest) {
// Custom buckets: [0, 1) [1, 2) [2, 3) [3, INT_MAX)
BucketRanges ranges(5);
diff --git a/base/metrics/sparse_histogram_unittest.cc b/base/metrics/sparse_histogram_unittest.cc
index 83cf5d37..7ad5558 100644
--- a/base/metrics/sparse_histogram_unittest.cc
+++ b/base/metrics/sparse_histogram_unittest.cc
@@ -81,6 +81,26 @@
EXPECT_EQ(25, snapshot2->GetCount(101));
}
+TEST_F(SparseHistogramTest, AddCount_LargeValuesDontOverflow) {
+ scoped_ptr<SparseHistogram> histogram(NewSparseHistogram("Sparse"));
+ scoped_ptr<HistogramSamples> snapshot(histogram->SnapshotSamples());
+ EXPECT_EQ(0, snapshot->TotalCount());
+ EXPECT_EQ(0, snapshot->sum());
+
+ histogram->AddCount(1000000000, 15);
+ scoped_ptr<HistogramSamples> snapshot1(histogram->SnapshotSamples());
+ EXPECT_EQ(15, snapshot1->TotalCount());
+ EXPECT_EQ(15, snapshot1->GetCount(1000000000));
+
+ histogram->AddCount(1000000000, 15);
+ histogram->AddCount(1010000000, 25);
+ scoped_ptr<HistogramSamples> snapshot2(histogram->SnapshotSamples());
+ EXPECT_EQ(55, snapshot2->TotalCount());
+ EXPECT_EQ(30, snapshot2->GetCount(1000000000));
+ EXPECT_EQ(25, snapshot2->GetCount(1010000000));
+ EXPECT_EQ(55250000000LL, snapshot2->sum());
+}
+
TEST_F(SparseHistogramTest, MacroBasicTest) {
UMA_HISTOGRAM_SPARSE_SLOWLY("Sparse", 100);
UMA_HISTOGRAM_SPARSE_SLOWLY("Sparse", 200);
