Copy only accessed PersistentHistogramData fields when validating.

base/metrics/persistent_histogram_allocator.cc

Index: base/metrics/persistent_histogram_allocator.cc
diff --git a/base/metrics/persistent_histogram_allocator.cc b/base/metrics/persistent_histogram_allocator.cc
index c73d8442987b0a82ea4df573254a2bf99094c900..30d28349ff35939f1070d748edef9a244aeabb5c 100644
--- a/base/metrics/persistent_histogram_allocator.cc
+++ b/base/metrics/persistent_histogram_allocator.cc
@@ -568,9 +568,18 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::CreateHistogram(
   // Copy the histogram_data to local storage because anything in persistent
   // memory cannot be trusted as it could be changed at any moment by a
   // malicious actor that shares access. The contents of histogram_data are
-  // validated below; the local copy is to ensure that the contents cannot
-  // be externally changed between validation and use.
-  PersistentHistogramData histogram_data = *histogram_data_ptr;
+  // validated below; the local copy of relevant fields is to ensure that the
+  // contents cannot be externally changed between validation and use. A direct
+  // copy isn't possible because of the counts_ref field that must be read
+  // using an atomic operation.
+  PersistentHistogramData histogram_data;

[Alexei Svitkine (slow), 2017/05/11 20:20:07]

Can you add a CopyTo() function instead? This way, someone changing the struct
can be sure to update it instead of the code living elsewhere (here).

[bcwhite, 2017/05/11 20:23:16]

I looked at that path...  but it was going to get complicated because the
metadata structures should also use atomic copies and they're not needed for
this code.

Perhaps I should remove the local PHD instance and create individual variables
instead?

[Alexei Svitkine (slow), 2017/05/11 20:27:55]

That SGTM.

[bcwhite, 2017/05/11 21:03:29]

Done.

+  histogram_data.histogram_type = histogram_data_ptr->histogram_type;
+  histogram_data.flags = histogram_data_ptr->flags;
+  histogram_data.minimum = histogram_data_ptr->minimum;
+  histogram_data.maximum = histogram_data_ptr->maximum;
+  histogram_data.bucket_count = histogram_data_ptr->bucket_count;
+  histogram_data.ranges_ref = histogram_data_ptr->ranges_ref;
+  histogram_data.ranges_checksum = histogram_data_ptr->ranges_checksum;
 
   HistogramBase::Sample* ranges_data =
       memory_allocator_->GetAsArray<HistogramBase::Sample>(
@@ -606,7 +615,7 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::CreateHistogram(
   size_t counts_bytes =
       CalculateRequiredCountsBytes(histogram_data.bucket_count);
   PersistentMemoryAllocator::Reference counts_ref =
-      subtle::NoBarrier_Load(&histogram_data.counts_ref);
+      subtle::NoBarrier_Load(&histogram_data_ptr->counts_ref);
   if (counts_bytes == 0 ||
       (counts_ref != 0 &&
        memory_allocator_->GetAllocSize(counts_ref) < counts_bytes)) {