Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(531)

Unified Diff: base/metrics/histogram_persistence.cc

Issue 1485763002: Merge multiple histogram snapshots into single one for reporting. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@shared-histograms
Patch Set: addressed remaining review comments by Alexei Created 4 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « base/metrics/histogram_base.cc ('k') | base/metrics/histogram_snapshot_manager.h » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: base/metrics/histogram_persistence.cc
diff --git a/base/metrics/histogram_persistence.cc b/base/metrics/histogram_persistence.cc
index ae7d48b77f8791545bce6a5e84e7b255d405e899..535d11409276ea117357676c64061ca7c3eb3f7b 100644
--- a/base/metrics/histogram_persistence.cc
+++ b/base/metrics/histogram_persistence.cc
@@ -76,6 +76,7 @@ struct PersistentHistogramData {
uint32_t ranges_checksum;
PersistentMemoryAllocator::Reference counts_ref;
HistogramSamples::Metadata samples_metadata;
+ HistogramSamples::Metadata logged_metadata;
// Space for the histogram name will be added during the actual allocation
// request. This must be the last field of the structure. A zero-size array
@@ -111,6 +112,22 @@ BucketRanges* CreateRangesFromData(HistogramBase::Sample* ranges_data,
return ranges.release();
}
+// Calculate the number of bytes required to store all of a histogram's
+// "counts". This will return zero (0) if |bucket_count| is not valid.
+size_t CalculateRequiredCountsBytes(size_t bucket_count) {
+ // 2 because each "sample count" also requires a backup "logged count"
+ // used for calculating the delta during snapshot operations.
+ const unsigned kBytesPerBucket = 2 * sizeof(HistogramBase::AtomicCount);
+
+ // If the |bucket_count| is such that it would overflow the return type,
+ // perhaps as the result of a malicious actor, then return zero to
+ // indicate the problem to the caller.
+ if (bucket_count > std::numeric_limits<uint32_t>::max() / kBytesPerBucket)
+ return 0;
+
+ return bucket_count * kBytesPerBucket;
+}
+
} // namespace
const Feature kPersistentHistogramsFeature{
@@ -244,14 +261,21 @@ HistogramBase* CreatePersistentHistogram(
HistogramBase::AtomicCount* counts_data =
allocator->GetAsObject<HistogramBase::AtomicCount>(
histogram_data.counts_ref, kTypeIdCountsArray);
- if (!counts_data ||
- allocator->GetAllocSize(histogram_data.counts_ref) <
- histogram_data.bucket_count * sizeof(HistogramBase::AtomicCount)) {
+ size_t counts_bytes =
+ CalculateRequiredCountsBytes(histogram_data.bucket_count);
+ if (!counts_data || !counts_bytes ||
+ allocator->GetAllocSize(histogram_data.counts_ref) < counts_bytes) {
RecordCreateHistogramResult(CREATE_HISTOGRAM_INVALID_COUNTS_ARRAY);
NOTREACHED();
return nullptr;
}
+ // After the main "counts" array is a second array using for storing what
+ // was previously logged. This is used to calculate the "delta" during
+ // snapshot operations.
+ HistogramBase::AtomicCount* logged_data =
+ counts_data + histogram_data.bucket_count;
+
std::string name(histogram_data_ptr->name);
HistogramBase* histogram = nullptr;
switch (histogram_data.histogram_type) {
@@ -262,8 +286,10 @@ HistogramBase* CreatePersistentHistogram(
histogram_data.maximum,
ranges,
counts_data,
+ logged_data,
histogram_data.bucket_count,
- &histogram_data_ptr->samples_metadata);
+ &histogram_data_ptr->samples_metadata,
+ &histogram_data_ptr->logged_metadata);
DCHECK(histogram);
break;
case LINEAR_HISTOGRAM:
@@ -273,8 +299,10 @@ HistogramBase* CreatePersistentHistogram(
histogram_data.maximum,
ranges,
counts_data,
+ logged_data,
histogram_data.bucket_count,
- &histogram_data_ptr->samples_metadata);
+ &histogram_data_ptr->samples_metadata,
+ &histogram_data_ptr->logged_metadata);
DCHECK(histogram);
break;
case BOOLEAN_HISTOGRAM:
@@ -282,7 +310,9 @@ HistogramBase* CreatePersistentHistogram(
name,
ranges,
counts_data,
- &histogram_data_ptr->samples_metadata);
+ logged_data,
+ &histogram_data_ptr->samples_metadata,
+ &histogram_data_ptr->logged_metadata);
DCHECK(histogram);
break;
case CUSTOM_HISTOGRAM:
@@ -290,8 +320,10 @@ HistogramBase* CreatePersistentHistogram(
name,
ranges,
counts_data,
+ logged_data,
histogram_data.bucket_count,
- &histogram_data_ptr->samples_metadata);
+ &histogram_data_ptr->samples_metadata,
+ &histogram_data_ptr->logged_metadata);
DCHECK(histogram);
break;
default:
@@ -375,16 +407,15 @@ HistogramBase* AllocatePersistentHistogram(
return nullptr;
}
+ // If CalculateRequiredCountsBytes() returns zero then the bucket_count
+ // was not valid.
size_t bucket_count = bucket_ranges->bucket_count();
- // An overflow such as this, perhaps as the result of a milicious actor,
- // could lead to writing beyond the allocation boundary and into other
- // memory. Just fail the allocation and let the caller deal with it.
- if (bucket_count > std::numeric_limits<int32_t>::max() /
- sizeof(HistogramBase::AtomicCount)) {
+ size_t counts_bytes = CalculateRequiredCountsBytes(bucket_count);
+ if (!counts_bytes) {
NOTREACHED();
return nullptr;
}
- size_t counts_bytes = bucket_count * sizeof(HistogramBase::AtomicCount);
+
size_t ranges_bytes = (bucket_count + 1) * sizeof(HistogramBase::Sample);
PersistentMemoryAllocator::Reference ranges_ref =
allocator->Allocate(ranges_bytes, kTypeIdRangesArray);
« no previous file with comments | « base/metrics/histogram_base.cc ('k') | base/metrics/histogram_snapshot_manager.h » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698